-
Notifications
You must be signed in to change notification settings - Fork 105
feat: Refactor NoteTag to contain an arbitrary u32
#2219
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
Conversation
partylikeits1983
left a comment
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.
Looks great! Definitely simplifies the mental model when creating note tags.
Only question is how will network notes now be picked up by the network transaction builder if the ntx builder doesn't know which notes are network notes or not by looking at the note tag?
| /// specific account. One example is a P2ID note that enforces that it can only be consumed by a | ||
| /// specific account ID. The tag for such a P2ID note should make it easy for the receiver to find |
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.
While this sentence is not wrong per-se, I find it misleading, because it's not the tag that enforces anything in the P2ID note, but the AccountId encoded in note inputs.
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, good feedback! Simplified to drop the "enforce" part. Not sure if it's much clearer though:
A note targeted at an account is a note that is intended or even enforced to be consumed by a specific account. One example is a P2ID note that can only be consumed by a specific account ID. The tag for such a note should make it easy for the receiver to find the note.
Great question! This would be done through a standardized attachment, i.e. So this PR may make sense to be merged in one go with the |
bobbinth
left a comment
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.
Looks good! Thank you!
| /// - For local execution ([`AccountStorageMode::Private`] or [`AccountStorageMode::Public`]), | ||
| /// the two most significant bits are set to `0b11`, which allows for any note type to be | ||
| /// used. The following 14 bits are set to the most significant bits of the account ID, and | ||
| /// the remaining 16 bits are set to 0. | ||
| /// the two most significant bits are set to `0b00`. The following 14 bits are set to the most | ||
| /// significant bits of the account ID, and the remaining 16 bits are set to 0. | ||
| /// - For network execution ([`AccountStorageMode::Network`]), the most significant bits are set | ||
| /// to `0b00` and the remaining bits are set to the 30 most significant bits of the account | ||
| /// ID. |
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 believe we left the 0b00 prefixes for simplicity and to limit the scope of this PR. I would consider removing them in subsequent PRs.
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.
To double-check, you suggest allowing up to 32 bits of an account ID in the note tag? Or asked another way: What would we replace the 0b00 bits with?
There are two conflicting "requirements":
- If we allow up to 32 bits in note tags, then it would make sense to increase
NoteTag::DEFAULT_NETWORK_ACCOUNT_TARGET_TAG_LENGTHfrom 30 to 32. - Note tag lengths in
RoutingParameterscan only encode up to30butAddress::with_routing_parametersrequires that network account tag length is set toNoteTag::DEFAULT_NETWORK_ACCOUNT_TARGET_TAG_LENGTH.
On the other hand, AccountStorageMode::Network isn't really needed anymore after the note tag and attachment refactor, since we'll have a different way to detect network account targets.
The other use of AccountStorageMode::Network is to detect network accounts in the node. We should be able to replace network account detection by checking for the presence of a "network account component" (e.g. via a named storage slot) to detect network accounts in the node, making it possible to drop this special storage mode.
Then we can simplify routing parameters and note tag construction.
So, I think the best course of action is:
- Finish note tag/attachment refactor. Replaces network account target detection.
- Implement a "network account component" that accounts can add to make them network accounts (or remove to no longer be network accounts). Replaces network account detection.
- This can hold a named storage slot with an array/map that contains the note scripts the account can consume, which we discussed a while ago.
- Remove
AccountStorageMode::Networkand simplify note tag construction and routing parameters. - Drop
0b00prefix from account target note tags and somehow (tbd) deal with the 32 / 30 discrepancy in routing parameters.
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.
To double-check, you suggest allowing up to 32 bits of an account ID in the note tag?
Yes, allowing full 32 bits for note tag - but after we finish note attachment refactoring. This way, we don't need to care about DEFAULT_NETWORK_ACCOUNT_TARGET_TAG_LENGTH, I think.
To summarize: I agree with the course of action you've outlined.
mmagician
left a comment
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.
LGTM, thanks!
* chore: Rename `create_random_note` to `*_default_*` and add pub helper * chore: replace create_mock_notes in test_epilogue * chore: Remove `create_mock_notes`
* feat: Define `NoteTag` to wrap arbitrary `u32` * feat: Rework note tag docs and constructors * chore: Remove use case constructors and refactor swap tag * feat: Remove note tag validation * feat: Rename account ID conversion to account target * feat: Update note tag docs * chore: add changelog * chore: Make `NoteMetadata::new` infallible * chore: address review comments on docs * chore: remove outdated note tag rules in metadata docs * chore: remove `create_mock_notes` (0xMiden#2236) * chore: Rename `create_random_note` to `*_default_*` and add pub helper * chore: replace create_mock_notes in test_epilogue * chore: Remove `create_mock_notes` --------- Co-authored-by: Bobbin Threadbare <[email protected]>
* feat: Define `NoteTag` to wrap arbitrary `u32` * feat: Rework note tag docs and constructors * chore: Remove use case constructors and refactor swap tag * feat: Remove note tag validation * feat: Rename account ID conversion to account target * feat: Update note tag docs * chore: add changelog * chore: Make `NoteMetadata::new` infallible * chore: address review comments on docs * chore: remove outdated note tag rules in metadata docs * chore: remove `create_mock_notes` (0xMiden#2236) * chore: Rename `create_random_note` to `*_default_*` and add pub helper * chore: replace create_mock_notes in test_epilogue * chore: Remove `create_mock_notes` --------- Co-authored-by: Bobbin Threadbare <[email protected]>
Refactors the
NoteTagto wrap an arbitraryu32.Main changes:
auxintoNoteAttachmentand simplifyNoteTag#2109 (comment) for the rationale.NoteTag.NoteExecutionMode.RoutingParameters, where we make use of the fact that note tags are never larger than 30. This could be done separately, or left as is. I don't have a strong opinion, but for simplicity am leaning towards leaving things as-is.from_account_idconstructor towith_account_targetto give the conventional "account target" note tag category a more precise name.with_custom_account_targetconstructor (previouslyfrom_local_account_id).AccountStorageMode::Networkto simplify the note tag andAddressfurther.docs/.NoteMetadata::newinfallible since it no longer needs to validate that tag.part of #2109