Flexible Minting Policies for Token Standards#2559
Flexible Minting Policies for Token Standards#2559onurinanc wants to merge 34 commits into0xMiden:nextfrom
Conversation
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! Not a full review yet - but I left some comments inline. The main comment so far is about splitting the component into a "policy manager" component and separate policy components (probably just a single policy component for now).
| #! | ||
| #! Invocation: exec | ||
| proc assert_valid_policy_root | ||
| dupw exec.word::eqz |
There was a problem hiding this comment.
We could use word::testz procedure here to avoid the dupw.
| exec.active_account::get_map_item | ||
| # => [ALLOWED_FLAG, MINT_POLICY_ROOT] | ||
|
|
||
| dupw exec.word::eqz not |
| const MINT_POLICY_PROC_ROOT_SLOT=word("miden::standards::mint_policies::proc_root") | ||
| const MINT_POLICY_PARAMS_SLOT=word("miden::standards::mint_policies::params") | ||
| const MINT_POLICY_ALLOWED_ROOTS_SLOT=word("miden::standards::mint_policies::allowed_roots") | ||
| const MINT_POLICY_PROC_ROOT_PTR=0 |
There was a problem hiding this comment.
Would be good to add some comments explaining the purpose and expected formats of the storage slots. For example, it is not immediately clear what "allowed roots" means.
crates/miden-standards/asm/standards/mint_policies/owner_controlled.masm
Show resolved
Hide resolved
|
All done! Please review: @bobbinth |
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
I left a few comments related to naming and structure, e.g. whether it should be MintPolicies or MintPolicyManager.
| # See the `BasicFungibleFaucet` Rust type's documentation for more details. | ||
|
|
||
| pub use ::miden::standards::faucets::basic_fungible::distribute | ||
| pub use ::miden::standards::faucets::basic_fungible::mint |
There was a problem hiding this comment.
That's a very welcome change 🙂
Since this does both minting (like miden::protocol::faucet::mint), but also creating a note, maybe we should consider mint_to_note as the name to reflect the note creation side effect in the name.
There was a problem hiding this comment.
@PhilippGackstatter I think the intent of mint_to_note is good but instead what about having mint_and_create_note which semantically sounds better?
There was a problem hiding this comment.
That's a bit more explicit and also a good choice. Another one might be mint_and_send (saw it in OpenZeppelin/miden-confidential-contracts#79 (comment)) which is a bit more concise but less explicit (does not mention note).
There was a problem hiding this comment.
I like mint_and_send - though, not a very strong preference.
And, in a future PR, we could also add a regular mint procedure which mints an asset and adds it to the account's vault.
| pub proc distribute | ||
| pub proc mint | ||
| exec.faucets::distribute |
There was a problem hiding this comment.
The faucets::distribute should probably also be renamed to mint, right?
| #! - note sender is not owner. | ||
| #! | ||
| #! Invocation: dynexec | ||
| pub proc mint_policy_owner_only |
There was a problem hiding this comment.
Nit: It might be sufficient to name this owner_only, since the module is called mint_policies already, so the module-qualified name would be mint_policies::owner_only which reads naturally.
| # Active policy root slot. | ||
| # Layout: [proc_root_w0, proc_root_w1, proc_root_w2, proc_root_w3] | ||
| const MINT_POLICY_PROC_ROOT_SLOT=word("miden::standards::mint_policies::proc_root") |
There was a problem hiding this comment.
| # Active policy root slot. | |
| # Layout: [proc_root_w0, proc_root_w1, proc_root_w2, proc_root_w3] | |
| const MINT_POLICY_PROC_ROOT_SLOT=word("miden::standards::mint_policies::proc_root") | |
| # Active policy root slot. | |
| # Layout: [PROC_ROOT] | |
| const MINT_POLICY_PROC_ROOT_SLOT=word("miden::standards::mint_policies::proc_root") |
Nit: Avoid specifying the individual word elements.
| exec.word::testz not | ||
| assert.err=ERR_MINT_POLICY_ROOT_NOT_ALLOWED | ||
| # => [ALLOWED_FLAG, MINT_POLICY_ROOT] | ||
|
|
||
| dropw | ||
| # => [MINT_POLICY_ROOT] |
There was a problem hiding this comment.
Nit: We can avoid the dropw by simplifying to word::eqz not assert (or assertz directly).
| #[derive(Debug, Clone, Copy)] | ||
| pub struct MintPolicies { | ||
| initial_policy_root: Word, | ||
| } |
There was a problem hiding this comment.
Should this be called MintPolicyManager and the mint_policies_library be renamed to mint_policy_manager_library along with crates/miden-standards/asm/account_components/faucets/mint_policies.masm?
I'd also suggest renaming MintPolicyConfig -> MintPolicy so we can "instantiate a mint policy manager with a mint policy".
| let allowed_roots = | ||
| if mint_policies.initial_policy_root == MintPolicies::owner_only_policy_root() { | ||
| StorageMap::with_entries([( | ||
| StorageMapKey::from_raw(MintPolicies::owner_only_policy_root()), | ||
| Word::from([1u32, 0, 0, 0]), | ||
| )]) | ||
| } else { | ||
| StorageMap::with_entries([ | ||
| ( | ||
| StorageMapKey::from_raw(MintPolicies::owner_only_policy_root()), | ||
| Word::from([1u32, 0, 0, 0]), | ||
| ), | ||
| ( | ||
| StorageMapKey::from_raw(mint_policies.initial_policy_root), | ||
| Word::from([1u32, 0, 0, 0]), | ||
| ), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Nit: Consider introducing a constant or variable that represents the "allowed flag" (word [1,0,0,0]).
| let mut rng = RpoRandomCoin::new([Felt::from(rng_seed); 4].into()); | ||
| let note = NoteBuilder::new(sender_account_id, &mut rng) | ||
| .note_type(NoteType::Private) | ||
| .tag(NoteTag::default().into()) | ||
| .serial_number(Word::from([rng_seed, rng_seed + 1, rng_seed + 2, rng_seed + 3])) | ||
| .code(note_script_code) | ||
| .build()?; |
There was a problem hiding this comment.
| let mut rng = RpoRandomCoin::new([Felt::from(rng_seed); 4].into()); | |
| let note = NoteBuilder::new(sender_account_id, &mut rng) | |
| .note_type(NoteType::Private) | |
| .tag(NoteTag::default().into()) | |
| .serial_number(Word::from([rng_seed, rng_seed + 1, rng_seed + 2, rng_seed + 3])) | |
| .code(note_script_code) | |
| .build()?; | |
| let mut rng = RpoRandomCoin::new([Felt::from(rng_seed); 4].into()); | |
| let note = NoteBuilder::new(sender_account_id, &mut rng) | |
| .note_type(NoteType::Private) | |
| .code(note_script_code) | |
| .build()?; |
Nit: The serial number is generated by the RNG internally and the note tag should already be set to its default by the builder.
The NoteBuilder can also compile a string itself so no need to use CodeBuilder above.
|
|
||
| let tx_context = mock_chain | ||
| .build_tx_context(faucet_id, &[], &[note])? | ||
| .add_note_script(note_script) |
There was a problem hiding this comment.
| .add_note_script(note_script) |
Nit: Haven't checked but this is likely unnecessary.
| /// Tests owner minting on a network faucet built from two separate components: | ||
| /// `NetworkFungibleFaucet` and `MintPolicies`. | ||
| #[tokio::test] | ||
| async fn test_network_faucet_owner_can_mint_with_separate_mint_policies_component() |
There was a problem hiding this comment.
Is this test effectively testing additional things than the other tests that use a network fungible faucet with MintPolicyConfig::OwnerOnly? I think it doesn't and if so, I'd remove it.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! Not a full review still, but I left some comments inline.
crates/miden-standards/asm/standards/mint_policies/policy_manager.masm
Outdated
Show resolved
Hide resolved
| #! Enforces active mint policy by dynamic execution. | ||
| #! | ||
| #! Inputs: [amount, tag, note_type, RECIPIENT, pad(9)] | ||
| #! Outputs: [amount, tag, note_type, RECIPIENT, pad(9)] |
There was a problem hiding this comment.
I think the motivation here is that we create a note right after calling this procedure, and so, duplicating all note inputs would be a bit wasteful. I don't really have a strong preference here.
But it does make me think that this procedure is specifically related to sending notes. So, maybe the name here should reflect this? Something like enforce_mint_and_send_policy?
| #! - active policy predicate fails. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc enforce_mint_policy |
There was a problem hiding this comment.
I actually don't mind enforce_ prefix, but also don't have anything against the execute_ prefix.
| .storage_mode(AccountStorageMode::Network) | ||
| .with_auth_component(auth_component) | ||
| .with_component(NetworkFungibleFaucet::new(symbol, decimals, max_supply, owner_account_id)?) | ||
| .with_component(MintPolicies::new(mint_policy_config)) |
There was a problem hiding this comment.
Once we have the Ownable2Step component, we'd need to add it here too, right?
crates/miden-standards/asm/standards/mint_policies/owner_controlled.masm
Show resolved
Hide resolved
|
Thank you for the review, all done! @bobbinth @PhilippGackstatter Please review. |
bobbinth
left a comment
There was a problem hiding this comment.
Looks great! Thank you! I think we are pretty close. I left some more comments inline. The main one is about potentially making MintPolicyManager into a stand-alone component. We could also decide to do it in a follow-up PR to merge this PR sooner.
| #! Invocation: exec | ||
| @locals(4) | ||
| pub proc distribute | ||
| pub proc mint_and_send |
There was a problem hiding this comment.
nit: I'd probably update the comment on line 38 above a bit as well.
There was a problem hiding this comment.
Not for this PR, but we should probably think about allowing updates to policy allowlist (i.e., adding or removing policies). Should be pretty easy to do - we'd probably need something like add_allowed_policy and remove_allowed_policy procedures.
There was a problem hiding this comment.
I believe this would be a parallel work to upgradable/updatable contracts. The main reason is that, even if we provide adding/removing allowed policies, if the account code is not upgradable, this won't make any sense since the corresponding code won't be the part of the account. This is the main reason why I didn't include updating the allowed policies in this PR. That means, when upgrading the account code, the user can also update the allowed policies. So, I think that there is no need for updating the allowed policies.
| # Keep this procedure digest distinct from network_fungible::mint_and_send. | ||
| push.0 drop |
There was a problem hiding this comment.
Why do we need to do this?
There was a problem hiding this comment.
@bobbinth The main problem is that when both NetworkFungibleFaucet and BasicFungibleFaucet have the same procedure digest (without having push.0 drop, both mint_and_send and burn will have the same procedure digests), and AccountComponentInterface::try_from is not able to differentiate those components, and we're having MissingNetworkFungibleFaucetInterface as the account component interface matches with BasicFungibleFaucet. We didn't have this problem before they produced different procedure digests. @PhilippGackstatter do you have any suggestion on this?
The first and the most basic solution is keeping the procedure digests different as adding push.0 drop
The second one is refactoring AccountComponentInterface
So, I would like to keep it simple for now, and you could potentially refactor the AccountComponentInterface
There was a problem hiding this comment.
I think that's an okay workaround for now, as we plan to remove the AccountComponentInterface as it is and then this problem would naturally go away. It would be good to leave a TODO: Remove once AccountComponentInterface is refactored here, though.
There was a problem hiding this comment.
I would mention somewhere in the comments at the top of the file that this component requires the policy manager component to also be in the account.
There was a problem hiding this comment.
Same comment as above re mentioning the policy manager dependency.
| pub use ::miden::standards::mint_policies::auth_tx_controlled::allow_all | ||
| pub use ::miden::standards::mint_policies::policy_manager::set_mint_policy | ||
| pub use ::miden::standards::mint_policies::policy_manager::get_mint_policy |
| pub fn create_network_fungible_faucet( | ||
| init_seed: [u8; 32], | ||
| symbol: TokenSymbol, | ||
| decimals: u8, | ||
| max_supply: Felt, | ||
| access_control: AccessControl, | ||
| ) -> Result<Account, FungibleFaucetError> { |
There was a problem hiding this comment.
Not for this PR, but we should probably make this take TokenMetadata instead of passing in symbol, decimals, and max_supply separately.
There was a problem hiding this comment.
I think TokenMetadata PR should address this one but if not I'll be addressing in the follow-up PR.
| @@ -300,6 +301,7 @@ | |||
| .with_auth_component(auth_component) | |||
| .with_component(NetworkFungibleFaucet::new(symbol, decimals, max_supply)?) | |||
| .with_component(access_control) | |||
There was a problem hiding this comment.
Before getting here, we should validate that access_control is Ownable2Step. I know this is the only variant right now, but as we add variants in the future, this could change. And since we here depend on Ownable2Step - we should make sure we check it.
| .with_description( | ||
| "Mint policy owner controlled component for network fungible faucets", | ||
| ) | ||
| .with_storage_schema(storage_schema); |
There was a problem hiding this comment.
In other places, we started to encapsulate this into a separate method. For example, here it could be:
impl OwnerControlled {
pub fn component_metadata() -> AccountComponentMetadata {
let storage_schema = StorageSchema::new(vec![
OwnerControlled::active_policy_proc_root_slot_schema(),
OwnerControlled::allowed_policy_proc_roots_slot_schema(),
OwnerControlled::policy_authority_slot_schema(),
])
.expect("storage schema should be valid");
AccountComponentMetadata::new(OwnerControlled::NAME, [AccountType::FungibleFaucet])
.with_description(
"Mint policy owner controlled component for network fungible faucets",
)
.with_storage_schema(storage_schema);
}
}And then here, this becomes just:
let metadata = OwnerControlled::component_metadata();| pub struct AuthTxControlled { | ||
| initial_policy_root: Word, | ||
| } |
There was a problem hiding this comment.
I'm wondering if the naming here is accurate - specifically the AuthTx part. Technically, we have authentication procedures - so, this could be AuthProcControlled - but I wonder if just AuthControlled is sufficient.
There was a problem hiding this comment.
I don't think AuthTxControlled constitutes a good naming but all the naming of the auth component account starts with pub proc auth_tx. However, AuthControlled would be a candidate since the owner-controlled one is basically NoAuth. But, there is also current change, which we specify the auth component procedures as @authscript, so maybe AuthScriptControlled, so I've found naming this one quite confusing.
CC: @PhilippGackstatter
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
I'll approve since most of my comments apply to follow-ups.
| pub use ::miden::standards::mint_policies::owner_controlled::owner_only | ||
| pub use ::miden::standards::mint_policies::policy_manager::set_mint_policy | ||
| pub use ::miden::standards::mint_policies::policy_manager::get_mint_policy |
There was a problem hiding this comment.
Generally agreed that it seems better to have a base MintPolicyManager component that can be extended in plug & play fashion with additional mint policy components.
Not a strong opinion whether we should do it in this PR or later.
| pub proc mint_and_send | ||
| exec.policy_manager::execute_mint_policy | ||
| # => [new_amount, new_tag, new_note_type, NEW_RECIPIENT, pad(9)] | ||
|
|
||
| exec.faucets::distribute | ||
| exec.faucets::mint_and_send | ||
| # => [note_idx, pad(15)] | ||
| end |
There was a problem hiding this comment.
The code of NetworkFungibleFaucet and BasicFungibleFaucet is now identical, so I think we should collapse these into just one type, i.e. BasicFungibleFaucet.
The difference between a network and a "local" faucet is now just a different configuration of the overall account, but the distinction no longer exists at the account component level, and so we don't need different code and account components.
If agreed, we should create an issue as a follow-up. This could also make the token metadata #2439 simpler.
There was a problem hiding this comment.
Overall, I agree we should address this issue, but I'm not sure if we should to this in a follow-up PR or wait a bit more if there would be a specific needs for the NetworkFungibleFaucet in the future so that we don't need to re-implement it again.
|
|
||
| # Allowlist map slot for policy roots. | ||
| # Map entries: [PROC_ROOT] -> [1, 0, 0, 0] | ||
| # This slot ensure `dynexec` runs only allowed mint-policy roots, not arbitrary procedures. |
There was a problem hiding this comment.
| # This slot ensure `dynexec` runs only allowed mint-policy roots, not arbitrary procedures. | |
| # This slot ensures the policy manager runs only allowed mint-policy roots, not arbitrary procedures. |
Nit: I'd rephrase this
| const ALLOWED_POLICY_PROC_ROOTS_SLOT=word("miden::standards::mint_policy_manager::allowed_policy_proc_roots") | ||
|
|
||
| # Policy authority slot. | ||
| # Layout: [POLICY_AUTHORITY, 0, 0, 0] |
There was a problem hiding this comment.
| # Layout: [POLICY_AUTHORITY, 0, 0, 0] | |
| # Layout: [policy_authority, 0, 0, 0] |
Nit: felts are lowercase
| exec.word::testz | ||
| assertz.err=ERR_MINT_POLICY_ROOT_NOT_ALLOWED | ||
| # => [ALLOWED_FLAG, MINT_POLICY_ROOT] | ||
|
|
||
| dropw | ||
| # => [MINT_POLICY_ROOT] |
There was a problem hiding this comment.
Nit: This could be word::eqz assertz to avoid the dropw.
| #! | ||
| #! Invocation: exec | ||
| @locals(4) | ||
| pub proc execute_mint_policy |
There was a problem hiding this comment.
This procedure validates the mint policy, but get_mint_policy does not. Depending on the perspective, both are valid approaches:
- We can say that since
set_mint_policyvalidates the procedure when setting, so the stored mint policy is valid. - The only exception to the above is that the account deployer could set an invalid root, and so we could also say that we should always validate the mint policy on retrieval.
So far, I think we have usually used the first approach, though this was mostly for component-internal data. I'm not sure how much damage a malicious deployer could do by setting an invalid mint policy root, probably not much other than making their own account unusable, so I'd go with approach 1.
This makes me wish we could let every account component have some on-chain validation of its storage at account creation time so we could always go with the first approach and provide stronger guarantees.
There was a problem hiding this comment.
I think this is a good point! I agree that we would have stronger guarantees with on-chain validation, but it also adds more complexity for account creation, and this also needs an execution order design and rules between the other account components. I'm not exactly sure which one is the right direction.
| // [num_trigger_procs, allow_unauthorized_output_notes, allow_unauthorized_input_notes, 0]. | ||
| // | ||
| // With 1 trigger procedure (distribute), allow_unauthorized_output_notes=false, and | ||
| // With 1 trigger procedure (mint), allow_unauthorized_output_notes=false, and |
There was a problem hiding this comment.
| // With 1 trigger procedure (mint), allow_unauthorized_output_notes=false, and | |
| // With 1 trigger procedure (mint_and_send), allow_unauthorized_output_notes=false, and |
|
|
||
| // The procedure root map should contain the distribute procedure root. | ||
| let distribute_root = BasicFungibleFaucet::distribute_digest(); | ||
| // The procedure root map should contain the mint procedure root. |
There was a problem hiding this comment.
| // The procedure root map should contain the mint procedure root. | |
| // The procedure root map should contain the mint_and_send procedure root. |
| let policy_authority_slot = StorageSlot::with_value( | ||
| AuthControlled::policy_authority_slot().clone(), | ||
| Word::from([0u32, 0, 0, 0]), | ||
| ); |
There was a problem hiding this comment.
I think we should introduce an enum for the policy authority, because it's otherwise hard to understand which zero represents the actual config and what that zero represents. Something like:
#[repr(u8)]
pub enum MintPolicyAuthority {
AuthControlled = 0,
OwnerControlled = 1,
}
impl From<MintPolicyAuthority> for StorageSlot { ... }
impl AuthControlled {
pub fn mint_policy_authority(&self) -> MintPolicyAuthority {
MintPolicyAuthority::AuthControlled
}
}And then here we'd have:
let policy_authority_slot = StorageSlot::from(auth_controlled.mint_policy_authority);| static ACTIVE_MINT_POLICY_PROC_ROOT_SLOT_NAME: LazyLock<StorageSlotName> = LazyLock::new(|| { | ||
| StorageSlotName::new("miden::standards::mint_policy_manager::active_policy_proc_root") | ||
| .expect("storage slot name should be valid") | ||
| }); | ||
| static ALLOWED_MINT_POLICY_PROC_ROOTS_SLOT_NAME: LazyLock<StorageSlotName> = LazyLock::new(|| { | ||
| StorageSlotName::new("miden::standards::mint_policy_manager::allowed_policy_proc_roots") | ||
| .expect("storage slot name should be valid") | ||
| }); | ||
| static POLICY_AUTHORITY_SLOT_NAME: LazyLock<StorageSlotName> = LazyLock::new(|| { | ||
| StorageSlotName::new("miden::standards::mint_policy_manager::policy_authority") | ||
| .expect("storage slot name should be valid") | ||
| }); |
There was a problem hiding this comment.
These definitions are duplicated in crates/miden-standards/src/account/mint_policies/auth_controlled.rs, would be great to deduplicate, but also fine to do as part of the MintPolicyManager refactor.
The "miden::standards::mint_policy_manager::policy_authority" slot could be defined on the previously proposed MintPolicyAuthority enum.
|
All done! The corresponding issues for the follow-up PRs are opened! @bobbinth @PhilippGackstatter |
This PR corresponds to the discussion for Miden Token Standards, overall, it implements flexible and modular minting policies: OpenZeppelin/miden-confidential-contracts#39
Design specification related to this document: OpenZeppelin/miden-confidential-contracts#39 (comment)
Please see additional design discussion here: OpenZeppelin/miden-confidential-contracts#39 (reply in thread)
[ ] Left to-do: I would like to rename this procedure as
pub proc mintas we have discussed with @bobbinth but I would like to double-check is there any other thoughts on this?