Refactor(account) introducing account id key#2495
Refactor(account) introducing account id key#2495swaploard wants to merge 10 commits into0xMiden:nextfrom
Conversation
|
Hi @swaploard, thanks for the PR! Some larger PRs were merged and now would be a good time to merge this PR. Can you merge next into this one? |
|
Sure
Sure, I will do that. |
|
@PhilippGackstatter, You can review it now. |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. I left some comments inline - most of them are small nits.
| /// The account ID encoded as a key for use in AccountTree and | ||
| /// advice maps in `TransactionAdviceInputs`. |
There was a problem hiding this comment.
nit: comment wrapping (we usually don't wrap comments before 100 chars).
| } | ||
|
|
||
| // SMT WORD REPRESENTATION | ||
| // ================================================================================================ |
There was a problem hiding this comment.
nit: inside impls, we usually use ----- for separators (instead of ======).
| // LEAF INDEX | ||
| // ================================================================================================ |
| // ADVICE MAP KEY | ||
| // ================================================================================================ |
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
nit: let's add TESTS section separator above this line.
| pub fn from_word(word: Word) -> AccountId { | ||
| AccountId::try_from_elements(word[KEY_SUFFIX_IDX], word[KEY_PREFIX_IDX]) | ||
| .expect("account tree should only contain valid IDs") | ||
| } |
There was a problem hiding this comment.
I think this should be try_from_word() and return an error if the conversion failed.
| /// Returns advice map key: | ||
| /// | ||
| /// `[suffix, prefix, 0, 0]` | ||
| pub fn to_advice_map_word(&self) -> Word { |
There was a problem hiding this comment.
Do we actually want to have different ways of encoding the ID into a word? Maybe we can align these to make sure we always use [0, 0, suffix, prefix] representation?
There was a problem hiding this comment.
We do not need this.
| pub fn account_id_to_smt_index(account_id: AccountId) -> LeafIndex<SMT_DEPTH> { | ||
| account_id_to_smt_key(account_id).into() | ||
| AccountIdKey::from(account_id).as_word().into() | ||
| } |
There was a problem hiding this comment.
Do we still need this function?
There was a problem hiding this comment.
No we dont need this
| pub fn leaf(&self) -> SmtLeaf { | ||
| if self.commitment == Word::empty() { | ||
| let leaf_idx = LeafIndex::from(account_id_to_smt_key(self.id)); | ||
| let leaf_idx = LeafIndex::from(AccountIdKey::from(self.id).as_word()); |
There was a problem hiding this comment.
Could this not be something like:
let leaf_idx = AccountIdKey::from(self.id).to_left_index();|
|
||
| // Read the account header elements from the advice map. | ||
| let account_id_key = TransactionAdviceInputs::account_id_map_key(account_id); | ||
| let account_id_key = AccountIdKey::from(account_id).as_word(); |
There was a problem hiding this comment.
nit: I'd probably do just:
let account_id_key = AccountIdKey::from(account_id)And then below, would do:
.get(&account_id_key.as_word())
3 participants
This PR addresses issue (#2443) by introducing a dedicated AccountIdKey type to unify and centralize all AccountId → SMT and advice-map key conversions across the account tree and transaction layers. Previously, this logic was implemented via multiple free/helper functions, leading to duplication and scattered layout assumptions. This change removes those functions and replaces them with a single, strongly-typed abstraction that encapsulates the canonical word representations, leaf index conversion, and advice map key format, improving type safety, maintainability, and clarity.