feat(standards): add guardian component#3125
Conversation
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks great! One minor comment about if its possible to remove pub from certain procedures in the "internal procedures" section of the guardian masm file.
| #! - is_sender_guardian is 1 if the note sender is the guardian, otherwise 0. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc is_sender_guardian |
There was a problem hiding this comment.
Just a question, could this be an internal procedure?
Also since this procedure is in the "internal procedures" section, maybe it makes sense to make it an internal procedure or move it lower in this file. This same comment applies to assert_sender_is_guardian procedure.
There was a problem hiding this comment.
Thank you for the catch! I think this procedure shouldn't be in the internal procedures part, instead it should be in the public procedures part. Then, the procedures exposed to the account component should be in the external procedures section.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me. Left a few optional comments.
| #! - the note sender is not the guardian. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc assert_sender_is_guardian |
There was a problem hiding this comment.
Nit: This procedure is also in the internal section -> move to "PUBLIC INTERFACE" section.
| # Detect explicit clear via the zero address. The zero address is not a valid account ID, so | ||
| # we must check for it before validating. | ||
| dup.1 eq.0 dup.1 eq.0 and | ||
| # => [is_zero_address, new_guardian_suffix, new_guardian_prefix, pad(14)] |
There was a problem hiding this comment.
Nit: Should we add account_id::testz (does not consume the inputs) to replace this? I think we could use this procedure in other places now or in the future, so I think that makes sense.
| push.0.0 | ||
| # => [0, 0, new_guardian_suffix, new_guardian_prefix, pad(14)] | ||
|
|
||
| movup.3 movup.3 | ||
| # => [new_guardian_suffix, new_guardian_prefix, 0, 0, pad(14)] | ||
|
|
||
| exec.save_guardian_info | ||
| # => [pad(16)] |
There was a problem hiding this comment.
Nit: Consider building the word in save_guardian_info.
| /// ```text | ||
| /// Word: [guardian_suffix, guardian_prefix, 0, 0] | ||
| /// word[0] word[1] word[2] word[3] | ||
| /// ``` |
There was a problem hiding this comment.
| /// ```text | |
| /// Word: [guardian_suffix, guardian_prefix, 0, 0] | |
| /// word[0] word[1] word[2] word[3] | |
| /// ``` | |
| /// ```text | |
| /// [guardian_suffix, guardian_prefix, 0, 0] | |
| /// ``` |
Nit: I think this matches the layout we use in most places.
| pub struct Guardian { | ||
| /// The current guardian. `None` when no guardian is assigned. | ||
| guardian: Option<AccountId>, | ||
| } |
There was a problem hiding this comment.
I think the name is fine in principle, but since it is technically distinct from the Miden Guardian, I wonder if we should choose a different name. Then again, this guardian will probably be the same entity as the Miden Guardian most of the time, so maybe that's okay?
There was a problem hiding this comment.
In my mind, Miden Guardian should use this component eventually for some use cases such as freezing procedures, but I'm not sure if it is planned to support this as a feature or not. I think @bobbinth was suggesting to change this name to something else.
| /// Reconstructs a [`Guardian`] from a raw storage word. | ||
| /// | ||
| /// Format: `[guardian_suffix, guardian_prefix, 0, 0]` | ||
| pub fn try_from_word(word: Word) -> Result<Self, GuardianError> { | ||
| let guardian = account_id_from_felt_pair(word[0], word[1]) | ||
| .map_err(GuardianError::InvalidGuardianId)?; | ||
|
|
||
| Ok(Self { guardian }) | ||
| } |
There was a problem hiding this comment.
I wonder if we should use the same layout as AccountIdKey. It's not technically a key, but the main advantage is that we could reuse the logic of that type in a few places (except that we allow zero IDs here, which the key doesn't) and also layout consistency.
| /// Returns the current guardian, or `None` if no guardian is assigned. | ||
| pub fn guardian(&self) -> Option<AccountId> { | ||
| self.guardian | ||
| } |
There was a problem hiding this comment.
Nit: guardian.guradian() is a bit redundant. Maybe this could be just id or account_id?
3 participants
Closes: #3103