refactor: change TransferPolicy from enum to struct#2974
Open
onurinanc wants to merge 3 commits into
Open
Conversation
bobbinth
reviewed
May 23, 2026
Contributor
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline.
Comment on lines
+47
to
+58
| /// 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 } | ||
| } |
Contributor
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.
Comment on lines
+65
to
+68
| /// Returns the [`AccountComponent`]s that must accompany this burn policy. | ||
| pub(crate) fn into_components(self) -> Vec<AccountComponent> { | ||
| self.components | ||
| } |
Contributor
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>;
...
}
Comment on lines
+50
to
+58
| 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 } | ||
| } |
Comment on lines
+98
to
+108
| 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 } | ||
| } |
Comment on lines
+166
to
+172
| /// 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. |
Contributor
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()?;
Contributor
There was a problem hiding this comment.
Agreed. I'd suggest a bon builder:
#[bon::bon]
impl TokenPolicyManager {
#[builder]
pub fn new(
#[builder(field)] allowed_mint_policies: BTreeMap<AccountProcedureRoot, MintPolicy>,
#[builder(field)] allowed_burn_policies: BTreeMap<AccountProcedureRoot, BurnPolicy>,
#[builder(field)] allowed_send_policies: BTreeMap<AccountProcedureRoot, TransferPolicy>,
#[builder(field)] allowed_receive_policies: BTreeMap<AccountProcedureRoot, TransferPolicy>,
active_mint_policy: MintPolicy,
active_burn_policy: BurnPolicy,
active_send_policy: TransferPolicy,
active_receive_policy: TransferPolicy,
) -> TokenPolicyManager {
let active_mint_policy_root = active_mint_policy.root();
// Similarly for other policies.
let policies = allowed_mint_policies
.into_iter()
.chain(iter::once((active_mint_policy.root(), active_mint_policy)))
.map(|(root, policy)| (root, PolicyConfig::from(policy)))
.collect();
// TODO: Special handling for merging send and receive policies so they correctly merge into
// a PolicyConfig with kinds: [PolicyKind::Send, PolicyKind::Receive].
Self {
active_mint_policy_root,
active_burn_policy_root: todo!(),
active_send_policy_root: todo!(),
active_receive_policy_root: todo!(),
policies,
}
}
}
impl<S: token_policy_manager_builder::State> TokenPolicyManagerBuilder<S> {
/// TODO
pub fn allowed_mint_policy(mut self, mint_policy: MintPolicy) -> Self {
self.allowed_mint_policies.insert(mint_policy.root(), mint_policy);
self
}
}
impl From<MintPolicy> for PolicyConfig {
fn from(mint_policy: MintPolicy) -> Self {
PolicyConfig {
components: mint_policy.into_components(),
kinds: BTreeSet::from_iter([PolicyKind::Mint]),
}
}
}
fn test() {
TokenPolicyManager::builder()
.active_mint_policy(MintPolicy::owner_only())
.allowed_mint_policy(MintPolicy::allow_all())
// ...
.build();
}Calling active_mint_policy or allowed_mint_policy makes it explicit if I want to overwrite an already set active mint policy or not, and it looks cleaner (I think we wouldn't need PolicyRegistration).
Comment on lines
+67
to
+78
| pub fn basic_blocklist() -> Self { | ||
| Self { | ||
| root: BasicBlocklist::root(), | ||
| components: vec![BasicBlocklist::default().into()], | ||
| } | ||
| } | ||
|
|
||
| /// Returns a basic-blocklist transfer policy seeded with the given initial blocked accounts. | ||
| pub fn basic_blocklist_with_initial<I>(blocked_accounts: I) -> Self | ||
| where | ||
| I: IntoIterator<Item = AccountId>, | ||
| { |
Contributor
There was a problem hiding this comment.
Nit: I'd rename basic_blocklist_with_initial to with_basic_blocklist_ids and basic_blocklist to empty_basic_blocklist.
| /// Returns a transfer policy that rejects transfers whose native account is not in the | ||
| /// `allowed_accounts` map. The provided [`AllowlistStorage`] seeds the initial allowlist | ||
| /// entries at component-construction time. | ||
| pub fn basic_allowlist(allow_list: AllowlistStorage) -> Self { |
Contributor
There was a problem hiding this comment.
Nit: To match my other suggestion, this could become with_basic_allowlist.
Comment on lines
+166
to
+172
| /// 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. |
Contributor
There was a problem hiding this comment.
Agreed. I'd suggest a bon builder:
#[bon::bon]
impl TokenPolicyManager {
#[builder]
pub fn new(
#[builder(field)] allowed_mint_policies: BTreeMap<AccountProcedureRoot, MintPolicy>,
#[builder(field)] allowed_burn_policies: BTreeMap<AccountProcedureRoot, BurnPolicy>,
#[builder(field)] allowed_send_policies: BTreeMap<AccountProcedureRoot, TransferPolicy>,
#[builder(field)] allowed_receive_policies: BTreeMap<AccountProcedureRoot, TransferPolicy>,
active_mint_policy: MintPolicy,
active_burn_policy: BurnPolicy,
active_send_policy: TransferPolicy,
active_receive_policy: TransferPolicy,
) -> TokenPolicyManager {
let active_mint_policy_root = active_mint_policy.root();
// Similarly for other policies.
let policies = allowed_mint_policies
.into_iter()
.chain(iter::once((active_mint_policy.root(), active_mint_policy)))
.map(|(root, policy)| (root, PolicyConfig::from(policy)))
.collect();
// TODO: Special handling for merging send and receive policies so they correctly merge into
// a PolicyConfig with kinds: [PolicyKind::Send, PolicyKind::Receive].
Self {
active_mint_policy_root,
active_burn_policy_root: todo!(),
active_send_policy_root: todo!(),
active_receive_policy_root: todo!(),
policies,
}
}
}
impl<S: token_policy_manager_builder::State> TokenPolicyManagerBuilder<S> {
/// TODO
pub fn allowed_mint_policy(mut self, mint_policy: MintPolicy) -> Self {
self.allowed_mint_policies.insert(mint_policy.root(), mint_policy);
self
}
}
impl From<MintPolicy> for PolicyConfig {
fn from(mint_policy: MintPolicy) -> Self {
PolicyConfig {
components: mint_policy.into_components(),
kinds: BTreeSet::from_iter([PolicyKind::Mint]),
}
}
}
fn test() {
TokenPolicyManager::builder()
.active_mint_policy(MintPolicy::owner_only())
.allowed_mint_policy(MintPolicy::allow_all())
// ...
.build();
}Calling active_mint_policy or allowed_mint_policy makes it explicit if I want to overwrite an already set active mint policy or not, and it looks cleaner (I think we wouldn't need PolicyRegistration).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Closes: #2929