refactor: change TransferPolicy from enum to struct#2974
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline.
| /// Returns a burn policy resolving to the provided procedure root. The corresponding | ||
| /// component(s) must be installed by the caller separately — this descriptor carries no | ||
| /// companion components. | ||
| pub fn custom(root: AccountProcedureRoot) -> Self { | ||
| Self { root, components: Vec::new() } | ||
| } | ||
|
|
||
| /// Returns a burn policy resolving to the provided procedure root and shipping the provided | ||
| /// companion components. | ||
| pub fn from_components(root: AccountProcedureRoot, components: Vec<AccountComponent>) -> Self { | ||
| Self { root, components } | ||
| } |
There was a problem hiding this comment.
I would collapse these two constructors into one so that users can construct custom policies and provide components for them (i.e., the current custom() constructor is probably not needed).
For the from_components() constructor (which maybe we should rename into custom()), I'd change the second parameter to take an iterator over items that an be converted to components. This way, any object that can be converted to a list of components could be passed as the second parameter.
Lastly, we should add validation that the provided procedure root is actually a root of some procedure in the provided components.
| /// Returns the [`AccountComponent`]s that must accompany this burn policy. | ||
| pub(crate) fn into_components(self) -> Vec<AccountComponent> { | ||
| self.components | ||
| } |
There was a problem hiding this comment.
In addition (or maybe instead of), I'd implement IntoIterator for this - e.g., something like:
impl IntoIterator for BurnPolicy {
type Item = AccountComponent;
type IntoIter = alloc::vec::IntoIter<AccountComponent>;
...
}| pub fn custom(root: AccountProcedureRoot) -> Self { | ||
| Self { root, components: Vec::new() } | ||
| } | ||
|
|
||
| /// Returns a mint policy resolving to the provided procedure root and shipping the provided | ||
| /// companion components. | ||
| pub fn from_components(root: AccountProcedureRoot, components: Vec<AccountComponent>) -> Self { | ||
| Self { root, components } | ||
| } |
| pub fn custom(root: AccountProcedureRoot) -> Self { | ||
| Self { root, components: Vec::new() } | ||
| } | ||
|
|
||
| /// Returns a transfer policy resolving to the provided procedure root and shipping the | ||
| /// provided companion components. Use this for fully bespoke policy compositions where the | ||
| /// caller wants the manager to install the companion components alongside the procedure | ||
| /// root. | ||
| pub fn from_components(root: AccountProcedureRoot, components: Vec<AccountComponent>) -> Self { | ||
| Self { root, components } | ||
| } |
| /// Construct via [`Self::new`] and chain the per-kind builders ([`Self::with_mint_policy`] / | ||
| /// [`Self::with_burn_policy`] / [`Self::with_send_policy`] / [`Self::with_receive_policy`]). | ||
| /// Each accepts a typed policy descriptor plus a [`PolicyRegistration`] flag to register the | ||
| /// policy as either the active one or as a reserved alternative for runtime switching via the | ||
| /// matching `set_*_policy` procedure. If [`PolicyRegistration::Active`] is supplied more than | ||
| /// once for the same kind, the most recent registration wins for the active slot — earlier | ||
| /// `Active` calls of that kind become reserved entries. |
There was a problem hiding this comment.
I'm not sure picking the most recent registration is the best approach here as it may lead to un-intended errors. But at the same time, it is good not to have to return an error from each setter. A potential way to do both is to introduce something like TokenPolicyManagerBuilder and then the way to use it would be:
let token_policy_manager = TokenPolicyManager::builder()
.with_mint_policy(MintPolicy::owner_only(), PolicyRegistration::Active)
.with_burn_policy(BurnPolicy::owner_only(), PolicyRegistration::Active)
.with_burn_policy(BurnPolicy::allow_all(), PolicyRegistration::Reserved)
.with_send_policy(TransferPolicy::allow_all(), PolicyRegistration::Active)
.with_receive_policy(TransferPolicy::allow_all(), PolicyRegistration::Active)
.build()?;2 participants
Closes: #2929