Skip to content
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

feat: Encode NoteExecutionHint into NoteMetadata #816

Merged
merged 19 commits into from
Aug 10, 2024
Merged

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Aug 5, 2024

Closes #582

@igamigo igamigo marked this pull request as ready for review August 7, 2024 15:56
@igamigo igamigo changed the title feat: Encode NoteExecutionHint into NoteMetadata feat: Encode NoteExecutionHint into NoteMetadata Aug 7, 2024
@bobbinth
Copy link
Contributor

bobbinth commented Aug 8, 2024

Is this ready for review or not yet?

@igamigo
Copy link
Collaborator Author

igamigo commented Aug 8, 2024

Is this ready for review or not yet?

Yes, sorry for the confusion

@bobbinth bobbinth self-requested a review August 8, 2024 19:10
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good! Not a full review yet, but I reviewed all of the non-test code and left some comments inline.

@bobbinth bobbinth mentioned this pull request Aug 9, 2024
3 tasks
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left a few small comments inline. After these are addressed, we can merge.

Comment on lines 279 to 286
# populate the metadata
# swap movup.4 movup.4
movup.3 movup.3 push.TWO_POW_38 mul add movup.2
# => [aux, encoded_type_and_ex_hint, tag]

# encode note_type and execution_hint into a single element
exec.account::get_id
# => [sender_acct_id, aux, encoded_type_and_ex_hint, tag]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple comments here:

  • Line 280 should probably be deleted.
  • Comment on line 284 should probably be moved to line 279.
  • A new comment should probably be written on line 284.

@bobbinth
Copy link
Contributor

bobbinth commented Aug 9, 2024

Now that #808 has been merged, some changes to MASM code here will be needed. @Fumuran - maybe you could make some suggestions on what needs to be updated?

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@bobbinth bobbinth merged commit 126d276 into next Aug 10, 2024
13 checks passed
@bobbinth bobbinth deleted the igamigo-encode-ex-hint branch August 10, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants