refactor: Extract Mint and Burn PolicyManager into their own component#2821
refactor: Extract Mint and Burn PolicyManager into their own component#2821onurinanc wants to merge 28 commits into0xMiden:nextfrom
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I think this basically covers the functionality we need. I left some comments inline - but they are mostly about naming.
It did take me a little time to wrap my head around how everything works - so, I wonder if there is a way to simplify things somehow - but I don't have concrete suggestions on this (maybe not using generics for policy manager could make it easier to understand - but I'm not sure about this).
| /// The name of the component. | ||
| pub const NAME: &'static str = "miden::standards::components::policies::mint::mod"; |
There was a problem hiding this comment.
Having mod in the component name feel a bit weird. I think is should probably be miden::standards::components::policies::mint::allow_all - but then the question is what should we name the procedure. One option could be check_policy (or something similar) and then the full procedure path would be:
miden::standards::components::policies::mint::allow_all::check_policy
| pub const NAME: &'static str = | ||
| "miden::standards::components::policies::burn::owner_controlled::owner_only"; | ||
|
|
||
| const PROC_NAME: &str = "owner_only"; |
There was a problem hiding this comment.
Related to the above comment, maybe the procedure name here should also be just check_policy or something similar?
| pub const NAME: &'static str = "miden::standards::components::policies::burn::mod"; | ||
|
|
||
| const PROC_NAME: &str = "allow_all"; |
There was a problem hiding this comment.
Related to the above comments. I'd probably rename the name to miden::standards::components::policies::burn::allow_all and the procedure name something like check_policy (or could be execute).
| /// Registers an additional policy root in the allowed-policies list. | ||
| pub fn with_allowed_policy(mut self, policy_root: Word) -> Self { |
There was a problem hiding this comment.
nit: I would probably mention in the comment that if the policy is already in the set, this is a noop.
| let account = AccountBuilder::new(init_seed) | ||
| .account_type(AccountType::FungibleFaucet) | ||
| .storage_mode(AccountStorageMode::Network) | ||
| .with_auth_component(auth_component) | ||
| .with_component(metadata) | ||
| .with_component(NetworkFungibleFaucet) | ||
| .with_component(access_control) | ||
| .with_component(MintOwnerControlled::owner_only()) | ||
| .with_component(BurnOwnerControlled::allow_all()) | ||
| .with_component(mint::PolicyManager::owner_controlled( | ||
| mint::owner_controlled::Config::OwnerOnly, | ||
| )) | ||
| .with_component(mint::owner_controlled::OwnerOnly) | ||
| .with_component(burn::PolicyManager::owner_controlled( | ||
| burn::owner_controlled::Config::AllowAll, | ||
| )) | ||
| .with_component(burn::owner_controlled::OwnerOnly) | ||
| .with_component(burn::AllowAll) |
There was a problem hiding this comment.
I think it may be worth adding a comment here explaining the dependency graph (hopefully, in the future we'll have this enforced, but since we don't have this yet, we should document it). The way I understand it, the graph looks like so:
NetworkFungibleFaucetrequiresmint::PolicyManagerandburn::PolicyManager.- These policy managers require policies, and are instantiated as follows:
mint::PolicyManageris instantiated withOwnerOnly.burn::PolicyManageris instantiated withAllowAllandOwnerOnly(withAllowAllbeing set by default).
OwnerOnlypolicies (for both mint and burn) requireOwnable2Step.
| const COMPONENT_NAME: &'static str = | ||
| "miden::standards::components::policies::mint::policy_manager"; |
There was a problem hiding this comment.
I don't think we need to have components in the component name - or at least I don't think we do this for other components. So, the name could be:
miden::standards::policies::mint::policy_manager
Or we could also make it more specific:
miden::standards::faucets::policies::mint::policy_manager
Also, we should make sure storage slot names are consistent with the component names. For the above component names, slot names would be:
miden::standards::policies::mint::policy_manager::policy_authority
miden::standards::policies::mint::policy_manager::active_policy_proc_root
miden::standards::policies::mint::policy_manager::allowed_policy_proc_roots
| const COMPONENT_NAME: &'static str = | ||
| "miden::standards::components::policies::burn::policy_manager"; |
There was a problem hiding this comment.
Same comment a above but for burn policies.
| /// The name of the component. | ||
| pub const NAME: &'static str = | ||
| "miden::standards::components::policies::mint::owner_controlled::owner_only"; |
There was a problem hiding this comment.
Same comment as above re using component in the name.
| /// policy is active when the faucet is first created. Future owner-controlled mint policies will | ||
| /// show up here as additional variants. | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub enum Config { |
There was a problem hiding this comment.
nit: Config feels a bit too generic here. I know we can use a module path, but I'd probably name this something like OwnerControlledPolicyConfig.
| /// policies so the owner can switch between them at runtime via `set_burn_policy`. This enum only | ||
| /// selects the **initial active** policy. | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub enum Config { |
There was a problem hiding this comment.
Same comment as above re using less generic name for Config.
2 participants
Related issue: #2615
Tracking issues:
MintPolicyManagerinto a stand-alone component OpenZeppelin/miden-confidential-contracts#84