-
Notifications
You must be signed in to change notification settings - Fork 107
feat(AggLayer): B2AGG note consumption check
#2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: agglayer
Are you sure you want to change the base?
Conversation
| # Ensure note attachment targets the consuming account. | ||
| exec.active_note::get_metadata | ||
| # => [NOTE_ATTACHMENT, METADATA_HEADER, pad(8)] | ||
|
|
||
| swapw dropw | ||
| # => [NOTE_ATTACHMENT, pad(12)] | ||
|
|
||
| # Reorder attachment word to [target_id_prefix, target_id_suffix, 0, 0]. | ||
| movup.2 movup.3 | ||
| # => [target_id_prefix, target_id_suffix, 0, 0, pad(12)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need ~3 helpers in miden::standards::network_account_target, something like:
const ATTACHMENT_SCHEME = NetworkAccountTarget::ATTACHMENT_SCHEMEget_id(NoteAttachment) -> AccountId- This asserts
attachment_scheme == NetworkAccountTarget::ATTACHMENT_SCHEME. - This assumes the attachment scheme was checked and simply returns the account ID. We may want to validate the account ID before returning?
- A getter for note execution hint may not be necessary.
- This asserts
new(AccountID, NoteExecutionMode) -> NoteAttachment- Looks like that's not necessary for this PR, but you're creating network notes in the bridge_out.masm using the note tag, which should be replaced with a call to this function.
Then you can just use network_account_target::get_id here. The constant is useful to add in case some other code wants to inspect the attachment scheme and branch on it instead of panicking (which is what get_id would do). I think this will be needed in other places so there is a benefit of moving this to miden::standards, and avoids low-level code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a good suggestion.
I created an issue to track this (PR already in draft), and for now added TODOs in the MASM code until that lands.
- make bridge a network account - move the ID check to non-reclaim branch
69ed247 to
bec68d1
Compare
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