Add AccountTree and PartialAccountTree
#1254
Conversation
|
@igamigo This should be a breaking change for the client - should I wait for anything before merging it? |
|
I think we can go ahead and merge this. We have not migrated to EDIT: The node's changes will very likely be breaking, maybe that's what you were referring to |
I was thinking that the account proof format changing and how it is fetched would be a breaking change, but maybe this is all abstracted away. If that's the case then all the better. |
|
I thought I could finish this PR today but I would still like to add more tests and refactor the |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I reviewed pretty much all non-test code and left some comments inline.
There was a problem hiding this comment.
Does AccountTree need to be in miden-base? Given that the NullifierTree is in miden-node, I think it would be more consistent to put the AccountTree there too. We could of course move the NullifierTree here too, but it seems like miden-node may be a more appropriate place for these structs (not a strong preference though).
There was a problem hiding this comment.
Thanks for raising this point. The main reason I wanted AccountTree in miden-base is so we can use it in MockChain. For the same reason, I'd like to move NullifierTree and ideally also Blockchain from the node to base. Otherwise, we're forced to reimplement those structures for mock chain and then we're more likely to have drifting implementations. For example, in the mock chain we currently have to reimplement low-level details like the nullifier layout:
Or for Blockchain, we should be able to deduplicate some code as well, e.g.:
- https://github.com/0xPolygonMiden/miden-node/blob/fd84a78ff0a998251eb738d9838e192d9ce4f4b7/crates/store/src/state.rs#L114
- https://github.com/0xPolygonMiden/miden-base/blob/7aeeb96f71a459b3209f956e47dd6521356044da/crates/miden-objects/src/testing/chain_mmr.rs#L16
It also means users who use MockChain have the same API, types and guarantees on those data structures as when interacting with the client or node. That's why having those core protocol data structures in miden-base is best, I think.
If we do this, then we'd have all of these consistently in miden-base:
AccountTreeandPartialAccountTreeNullifierTreeandPartialNullifierTreeBlockchainandPartialBlockchain(if we renameChainMmr).
I'm happy to open an issue to move the NullifierTree and Blockchain to base if you agree.
There was a problem hiding this comment.
Makes sense. Let's move NullifierTree and Blockchain here in a separate PR.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment which can be addressed in a follow-up PR.
There was a problem hiding this comment.
Makes sense. Let's move NullifierTree and Blockchain here in a separate PR.
| for (leaf_idx, leaf) in smt.leaves() { | ||
| if leaf.num_entries() >= 2 { | ||
| // SAFETY: Since we only inserted account IDs into the SMT, it is guaranteed that | ||
| // the leaf_idx is a valid Felt as well as a valid account ID prefix. | ||
| return Err(AccountTreeError::DuplicateIdPrefix { | ||
| duplicate_prefix: AccountIdPrefix::new_unchecked( | ||
| Felt::try_from(leaf_idx.value()) | ||
| .expect("leaf index should be a valid felt"), | ||
| ), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
We could check if the number of leaves is different from the number of passed in accounts before iterating over the leaves. This way, we'd iterate over leaves only if we know there was an issue.
Let's make this change in a follow-up PR.
4 participants
Adds strongly typed wrapper around
SmtforAccountTreeandPartialAccountTree.Adds a test to check that foreign accounts whose commitment is stale results in an error.
Added two tests that check the subtleties of the implementation.
Docs do not need to be updated after this change. The PR fixes an incorrect image link in the docs however.
closes #1157
closes #1158