feat(standards): unify MINT/BURN notes for fungible and non-fungible faucets via CodeInspection#3222
feat(standards): unify MINT/BURN notes for fungible and non-fungible faucets via CodeInspection#3222onurinanc wants to merge 7 commits into
Conversation
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
| impl AssetStatus { | ||
| /// Builds an [`AssetStatus`] from the raw status code held in element 0 of the registry value | ||
| /// word (0 = not issued, 1 = issued, 2 = burned). | ||
| fn from_status_code(status: Felt) -> Result<Self, NonFungibleFaucetError> { | ||
| match status.as_canonical_u64() { |
There was a problem hiding this comment.
nit: I would change this to TryFrom<u8> impl to match most other flat enums in the codebase which encode to and from a u8.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few small comments inline.
| tag: Felt, | ||
| }, | ||
| Public { | ||
| FungiblePublic { | ||
| recipient: NoteRecipient, | ||
| asset: FungibleAsset, | ||
| tag: Felt, |
There was a problem hiding this comment.
Not from this PR, but since we use NoteTag for non-fungible assets, I think we should use NoteTag for fungible assets as well (for the tag field).
| /// For fungible variants this is derived from the embedded asset; for non-fungible variants it | ||
| /// is the explicitly provided target faucet ID. | ||
| pub fn faucet_id(&self) -> AccountId { |
There was a problem hiding this comment.
Is this comment correct? I think in both cases we get the faucet ID from the asset.
| pub fn asset_status( | ||
| storage: &AccountStorage, | ||
| commitment: Word, | ||
| ) -> Result<AssetStatus, NonFungibleFaucetError> { |
There was a problem hiding this comment.
A couple of nits:
- I would change
commitmenttoasset_commitmentto make things a bit more clear. - I would probably rename this method to
get_asset_status().
| /// [`compute_asset_commitment`]: NonFungibleFaucet::compute_asset_commitment | ||
| pub fn asset_status( |
There was a problem hiding this comment.
The comment on line 318 seems disconnected from the rest of the comment. Should it be integrated somehow into it?
| if.true | ||
| # public output note creation | ||
| # ========================================================================================= | ||
| # FUNGIBLE MINT |
There was a problem hiding this comment.
The code below is a bit difficult to follow. I would probably split this into 4 helper procedures and then use the if-statements here to select which one of the four procedures to execute.
There was a problem hiding this comment.
agreed - I'd even take this a step further and have the fungible / non-fungible helpers separated out to their own files, e.g.:
crates/miden-standards/asm/standards/notes/mint.masmcrates/miden-standards/asm/standards/notes/mint_fungible.masmcrates/miden-standards/asm/standards/notes/mint_non_fungible.masm
Which sort of goes back to the old file structure, but IMO the separation would be clearer if the aggregating mint.masm only contained the minimal branching logic, with the per-faucet-type impl still in their own file
There was a problem hiding this comment.
plus, that would make the diff much smaller AFAICS
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo code organization ✅
| # Determine the faucet kind | ||
| procref.fungible::receive_and_burn | ||
| # => [FUNGIBLE_RECEIVE_AND_BURN_ROOT, pad(16)] | ||
|
|
||
| call.code_inspection::has_procedure | ||
| # => [is_fungible, pad(19)] | ||
|
|
||
| movdn.4 dropw | ||
| # => [is_fungible, pad(15)] | ||
|
|
||
| if.true | ||
| # Fungible faucet: burn the fungible asset. | ||
| call.fungible::receive_and_burn | ||
| # => [pad(16)] | ||
| else |
| if.true | ||
| # public output note creation | ||
| # ========================================================================================= | ||
| # FUNGIBLE MINT |
There was a problem hiding this comment.
agreed - I'd even take this a step further and have the fungible / non-fungible helpers separated out to their own files, e.g.:
crates/miden-standards/asm/standards/notes/mint.masmcrates/miden-standards/asm/standards/notes/mint_fungible.masmcrates/miden-standards/asm/standards/notes/mint_non_fungible.masm
Which sort of goes back to the old file structure, but IMO the separation would be clearer if the aggregating mint.masm only contained the minimal branching logic, with the per-faucet-type impl still in their own file
| if.true | ||
| # public output note creation | ||
| # ========================================================================================= | ||
| # FUNGIBLE MINT |
There was a problem hiding this comment.
plus, that would make the diff much smaller AFAICS
4 participants
Closes: #3163