feat(AggLayer): B2AGG note consumption check#2334
feat(AggLayer): B2AGG note consumption check#2334mmagician merged 41 commits intoagglayer-fixed-2from
B2AGG note consumption check#2334Conversation
mmagician
added
the
agglayer
mmagician
force-pushed
the
mmagician-bagg-check
branch
from
69ed247 to
bec68d1
Compare
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me (only reviewed the note attachment parts - let me know if you want me to review everything).
mmagician
force-pushed
the
mmagician-bagg-check
branch
from
36ddfdb to
84bfa0a
Compare
- make bridge a network account - move the ID check to non-reclaim branch
Co-authored-by: marti <marti@hungrycats.studio>
Co-authored-by: marti <marti@hungrycats.studio>
Co-authored-by: marti <marti@hungrycats.studio>
Co-authored-by: marti <marti@hungrycats.studio>
Co-authored-by: marti <marti@hungrycats.studio>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Looks good, similar to what @PhilippGackstatter mentioned above, create_b2agg_note will need to be updated as per #2283
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few comments inline - some of them could be done in a follow-up (but let's make sure these are captured in an issue).
crates/miden-standards/asm/standards/attachments/network_account_target.masm
Outdated
Show resolved
Hide resolved
| exec.get_id | ||
| # => [target_id_prefix, target_id_suffix] |
There was a problem hiding this comment.
I believe this will panic if the attachment is of incorrect kind/scheme right? Is that the desired behavior? It may be better to check if the attachment is correct (e.g., via something like is_network_account_target procedure) and extract the id only then.
If panic is the intended behavior, we should mention this in the doc comments for this procedure.
There was a problem hiding this comment.
I ended up re-working network_account_target.masm a little, so that now we have:
is_network_account_target(scheme, kind) -> boolget_id(ATTACHMENT) -> (prefix, suffix)(no validation of scheme/kind, simply a helper to extract id from attachment Word)active_account_matches_target_account() -> bool(panics if scheme/kind dont match)
cc @PhilippGackstatter who originally suggested #2337
There was a problem hiding this comment.
If this approach works better in practice, then we should use that 👍
This reverts commit 0282672.
crates/miden-standards/asm/standards/attachments/network_account_target.masm
Outdated
Show resolved
Hide resolved
| #! Inputs: [account_id_prefix, account_id_suffix, exec_hint] | ||
| #! Outputs: [attachment_scheme, attachment_kind, NOTE_ATTACHMENT] | ||
| #! Outputs: [attachment_kind, attachment_scheme, NOTE_ATTACHMENT] | ||
| #! | ||
| #! Where: | ||
| #! - account_id_{prefix,suffix} are the prefix and suffix felts of an account ID. | ||
| #! - exec_hint is the execution hint for the note. | ||
| #! - attachment_kind is the attachment kind (Word = 1) for use with `output_note::set_attachment`. | ||
| #! - attachment_scheme is the attachment scheme (1) for use with `output_note::set_attachment`. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc new | ||
| movup.2 | ||
| push.0 | ||
| push.NETWORK_ACCOUNT_TARGET_ATTACHMENT_KIND | ||
| push.NETWORK_ACCOUNT_TARGET_ATTACHMENT_SCHEME | ||
| # => [attachment_scheme, attachment_kind, ATTACHMENT] | ||
| push.NETWORK_ACCOUNT_TARGET_ATTACHMENT_KIND | ||
| # => [attachment_kind, attachment_scheme, ATTACHMENT] | ||
| end | ||
|
|
||
| #! Returns a boolean indicating whether the active account matches the target account | ||
| #! encoded in the active note's attachment. | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [is_equal] | ||
| #! | ||
| #! Where: | ||
| #! - is_equal is a boolean indicating whether the active account matches the target account. | ||
| #! | ||
| #! Panics if: | ||
| #! - the attachment is not a valid network account target. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc active_account_matches_target_account |
There was a problem hiding this comment.
Nit: The name suggests it returns only a bool but it also asserts things. Maybe it would make sense to call it assert_active_account_matches_target_account and take the ID as an input and do the equality assertion internally. This makes the error message worse due to less context, so not strictly an improvement.
There was a problem hiding this comment.
Agreed that the current name is slightly misleading.
I think we could change the name to assert_active_account_matches_target_account as you suggest, but actually without modifying the signature?
Specifically the active_account part of the name makes it clear to me that we are talking about the concrete account ID, so no need to pass ID as input, WDYT?
There was a problem hiding this comment.
I considered that as well, but thought the procedure would then not actually do what it suggests it does, i.e. it does not assert that the active account matches the target account, it only returns a bool. So, slight preference to do the assertion internally if that works for the call sites, but not a strong opinion at all.
There was a problem hiding this comment.
ok, in that case I'll leave it as-is
As per the discussion here, the network target account ID is placed into
NoteAttachmentinstead.While at it, I created a helper function, similarly to the one we have for
CLAIMnotes:create_b2agg_note, and refactored the tests to use that utility.TODO:
UPDATE_GERnote (once feat(AggLayer):UPDATE_GERnote #2333 lands)NoteAttachmentSchemeshould be used?closes #2173
closes #2189