-
Notifications
You must be signed in to change notification settings - Fork 166
fix(standards): report every storage size a standard note accepts #3810
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e72316f
fix(standards): report every storage size a standard note accepts
onurinanc 5e3a2f4
chore: link the changelog entry to the PR
onurinanc 570997d
fix comments
onurinanc 7c58eea
Merge remote-tracking branch 'origin' into fix-expected-storage-size
onurinanc 03dea5a
refactor(standards): represent accepted note storage sizes as an enum
onurinanc 9b8927d
test(standards): drop the MINT storage size parity test
onurinanc 067d055
refactor(standards): rename the NumStorageItems union variant to AnyOf
onurinanc 9b628ac
refactor(standards): define the accepted storage sizes on the note types
onurinanc d136160
chore: drop the changelog entries duplicated by the merge
onurinanc acd5e56
Merge remote-tracking branch 'origin' into fix-expected-storage-size
onurinanc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,30 +187,39 @@ impl StandardNote { | |
| } | ||
| } | ||
|
|
||
| /// Returns the expected number of storage items of the active note. | ||
| pub fn expected_num_storage_items(&self) -> usize { | ||
| /// Returns the [`NumStorageItems`] items this kind of note accepts. | ||
| pub fn num_storage_items(&self) -> NumStorageItems { | ||
| match self { | ||
| Self::P2ID => P2idNote::NUM_STORAGE_ITEMS, | ||
| Self::P2IDE => P2ideNote::NUM_STORAGE_ITEMS, | ||
| Self::SWAP => SwapNote::NUM_STORAGE_ITEMS, | ||
| Self::PSWAP => PswapNote::NUM_STORAGE_ITEMS, | ||
| Self::MINT => MintNote::NUM_STORAGE_ITEMS_PRIVATE, | ||
| Self::BURN => BurnNote::NUM_STORAGE_ITEMS, | ||
| Self::CONSTANT_FEE_POLICY_CONFIG => ConstantFeePolicyConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::FAUCET_POLICY_CONFIG => FaucetPolicyConfigNote::NUM_STORAGE_ITEMS, | ||
| // FaucetMetadataConfig storage is variable per action; this returns the upper bound. | ||
| Self::FAUCET_METADATA_CONFIG => FaucetMetadataConfigNote::MAX_NUM_STORAGE_ITEMS, | ||
| Self::MIN_BURN_AMOUNT_CONFIG => MinBurnAmountConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::ALLOWLIST_CONFIG => AllowlistConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::BLOCKLIST_CONFIG => BlocklistConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::PAUSE_CONFIG => PauseConfigNote::NUM_STORAGE_ITEMS, | ||
| // OwnerConfig storage is variable per action; this returns the upper bound. | ||
| Self::OWNER_CONFIG => OwnerConfigNote::MAX_NUM_STORAGE_ITEMS, | ||
| // RbacConfig storage is variable per action; this returns the upper bound. | ||
| Self::RBAC_CONFIG => RbacConfigNote::MAX_NUM_STORAGE_ITEMS, | ||
| Self::NETWORK_ACCOUNT_CONFIG => NetworkAccountConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::FEE_SPONSORSHIP => FeeSponsorshipNote::NUM_STORAGE_ITEMS, | ||
| Self::TX_FEE => TxFeeNote::NUM_STORAGE_ITEMS, | ||
| Self::P2ID => NumStorageItems::Exact(P2idNote::NUM_STORAGE_ITEMS), | ||
| Self::P2IDE => NumStorageItems::Exact(P2ideNote::NUM_STORAGE_ITEMS), | ||
| Self::SWAP => NumStorageItems::Exact(SwapNote::NUM_STORAGE_ITEMS), | ||
| Self::PSWAP => NumStorageItems::Exact(PswapNote::NUM_STORAGE_ITEMS), | ||
| Self::MINT => MintNote::NUM_STORAGE_ITEMS, | ||
| Self::BURN => NumStorageItems::Exact(BurnNote::NUM_STORAGE_ITEMS), | ||
| Self::CONSTANT_FEE_POLICY_CONFIG => { | ||
| NumStorageItems::Exact(ConstantFeePolicyConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::FAUCET_POLICY_CONFIG => { | ||
| NumStorageItems::Exact(FaucetPolicyConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::FAUCET_METADATA_CONFIG => FaucetMetadataConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::MIN_BURN_AMOUNT_CONFIG => { | ||
| NumStorageItems::Exact(MinBurnAmountConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::ALLOWLIST_CONFIG => { | ||
| NumStorageItems::Exact(AllowlistConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::BLOCKLIST_CONFIG => { | ||
| NumStorageItems::Exact(BlocklistConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::PAUSE_CONFIG => NumStorageItems::Exact(PauseConfigNote::NUM_STORAGE_ITEMS), | ||
| Self::OWNER_CONFIG => OwnerConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::RBAC_CONFIG => RbacConfigNote::NUM_STORAGE_ITEMS, | ||
| Self::NETWORK_ACCOUNT_CONFIG => { | ||
| NumStorageItems::Exact(NetworkAccountConfigNote::NUM_STORAGE_ITEMS) | ||
| }, | ||
| Self::FEE_SPONSORSHIP => NumStorageItems::Exact(FeeSponsorshipNote::NUM_STORAGE_ITEMS), | ||
| Self::TX_FEE => NumStorageItems::Exact(TxFeeNote::NUM_STORAGE_ITEMS), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -384,6 +393,37 @@ impl StandardNote { | |
| } | ||
| } | ||
|
|
||
| // NUM STORAGE ITEMS | ||
| // ================================================================================================ | ||
|
|
||
| /// The number of storage items a [`StandardNote`] accepts. | ||
| /// | ||
| /// A note script asserts the size of the storage it is handed, and some scripts accept more than | ||
| /// one size: they branch on it, or hold a variable-length tail. This is the set of sizes one of | ||
| /// them accepts, so that a caller can check a note against it instead of comparing against a | ||
| /// single constant. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum NumStorageItems { | ||
| /// The note holds exactly this many storage items. | ||
| Exact(usize), | ||
| /// The note holds any number of storage items in this inclusive range. | ||
| Range { min: usize, max: usize }, | ||
| /// The note holds a number of storage items accepted by any of these, and by none of the | ||
| /// sizes in between them. | ||
| AnyOf(&'static [NumStorageItems]), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, this is clever! |
||
| } | ||
|
|
||
| impl NumStorageItems { | ||
| /// Returns `true` if `num_items` is one of the accepted numbers of storage items. | ||
| pub fn accepts(&self, num_items: usize) -> bool { | ||
| match self { | ||
| Self::Exact(expected) => num_items == *expected, | ||
| Self::Range { min, max } => (*min..=*max).contains(&num_items), | ||
| Self::AnyOf(accepted) => accepted.iter().any(|accepted| accepted.accepts(num_items)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // HELPER FUNCTIONS | ||
| // ================================================================================================ | ||
|
|
||
|
|
@@ -449,3 +489,77 @@ impl Clone for NoteConsumptionStatus { | |
| } | ||
| } | ||
| } | ||
|
|
||
| // TESTS | ||
| // ================================================================================================ | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use miden_protocol::MAX_NOTE_STORAGE_ITEMS; | ||
|
|
||
| use super::*; | ||
|
|
||
| /// A MINT note holds exactly 13 items when it creates a private output note, and 20 or more | ||
| /// when it creates a public one, so the sizes in between are the only invalid ones below the | ||
| /// protocol limit. | ||
| #[test] | ||
| fn mint_accepts_both_the_private_and_the_public_storage_sizes() { | ||
| for num_items in [MintNote::NUM_STORAGE_ITEMS_PRIVATE, 20, 21, MAX_NOTE_STORAGE_ITEMS] { | ||
| assert!( | ||
| StandardNote::MINT.num_storage_items().accepts(num_items), | ||
| "{num_items} items should be accepted" | ||
| ); | ||
| } | ||
|
|
||
| for num_items in [0, 12, 14, 19, MAX_NOTE_STORAGE_ITEMS + 1] { | ||
| assert!( | ||
| !StandardNote::MINT.num_storage_items().accepts(num_items), | ||
| "{num_items} items should be rejected" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The config notes size their storage per action, and the sizes no action uses must be | ||
| /// rejected even when they fall between the bounds. | ||
| #[test] | ||
| fn config_notes_accept_only_the_sizes_their_actions_use() { | ||
| for (note, accepted, rejected) in [ | ||
| (StandardNote::OWNER_CONFIG, [1, 3].as_slice(), [0, 2, 4].as_slice()), | ||
| (StandardNote::RBAC_CONFIG, [2, 3, 4].as_slice(), [0, 1, 5].as_slice()), | ||
| ( | ||
| StandardNote::FAUCET_METADATA_CONFIG, | ||
| [2, 32].as_slice(), | ||
| [0, 3, 31, 33].as_slice(), | ||
| ), | ||
| ] { | ||
| for &num_items in accepted { | ||
| assert!( | ||
| note.num_storage_items().accepts(num_items), | ||
| "{} should accept {num_items} items", | ||
| note.name() | ||
| ); | ||
| } | ||
|
|
||
| for &num_items in rejected { | ||
| assert!( | ||
| !note.num_storage_items().accepts(num_items), | ||
| "{} should reject {num_items} items", | ||
| note.name() | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A note of fixed layout reports its size as exact, so no other size is accepted. | ||
| #[test] | ||
| fn fixed_size_notes_report_an_exact_size() { | ||
| for (note, num_items) in [ | ||
| (StandardNote::P2ID, P2idNote::NUM_STORAGE_ITEMS), | ||
| (StandardNote::P2IDE, P2ideNote::NUM_STORAGE_ITEMS), | ||
| (StandardNote::TX_FEE, TxFeeNote::NUM_STORAGE_ITEMS), | ||
| ] { | ||
| assert_eq!(note.num_storage_items(), NumStorageItems::Exact(num_items)); | ||
| assert!(!note.num_storage_items().accepts(num_items + 1)); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we change the
NUM_STORAGE_ITEMSconstant to be aNumStorageItems, then this dispatch could be a lot cleaner, see my other comment.