feat(standards): add flexible Burn Policies for Regulated Fungible Tokens

[onurinanc]

Related Issue: #2638, OpenZeppelin/miden-confidential-contracts#88

We currently have MintPolicies in place (without having the MintPolicyManager as a separate account component), and in this PR we introduce BurnPolicies while preserving the same overall structure.

Follow-up PR: Similar to MintPolicyManager for the mint policies, we will include BurnPolicyManager in the follow-up PR when introducing role-based access control.

[PhilippGackstatter]

Looks good!

I have a few main comments:

• I think the burn policy should not return the asset it takes as input, since it can't do anything useful with it, but correct me if I'm wrong.
• There is a lot of duplication:
  • We should be able to deduplicate the greatest amount of Rust code, i.e. OwnerControlled and AuthControlled.
  • I didn't find a good opportunity to deduplicate the mint and burn MASM policy managers, but if there is one, we should probably do it.
• May be worth double-checking that all procedures in the mint and burn policies modules that have Invocation: dynexec or Invocation: exec do not have pad comments, so we don't accidentally start overwriting these in the future (since the pad elements aren't actually garbage).

[PhilippGackstatter]

It looks like this is basically a copy of the mint policies OwnerControlled and similarly for AuthControlled, with the main difference being that it uses a different slot name? If there is no meaningful difference between the two, I wouldn't duplicate these but try to come up with a structure that accounts for the shared code. Maybe something like this:

// We should be able to make this type purely a private helper type now.
pub(super) struct OwnerControlled {
    initial_policy_root: Word,
}

pub struct MintOwnerControlled(OwnerControlled);
pub struct BurnOwnerControlled(OwnerControlled);

impl MintOwnerControlled {
    pub const NAME: &'static str =
        "miden::standards::components::mint_policies::owner_controlled";
}

impl BurnOwnerControlled {
    pub const NAME: &'static str =
        "miden::standards::components::burn_policies::owner_controlled";
}

impl From<MintOwnerControlled> for AccountComponent {
    fn from(mint_owner_controlled: MintOwnerControlled) -> Self {
        // The encoding logic lives in OwnerControlled, and we just pass different slot names
        // here.
        let storage_slots = mint_owner_controlled.0.to_storage_slots(
            MintOwnerControlled::allowed_policy_proc_roots_slot(),
            MintOwnerControlled::active_policy_proc_root_slot(),
        );

        todo!()
    }
}

impl From<BurnOwnerControlled> for AccountComponent {
    fn from(burn_owner_controlled: BurnOwnerControlled) -> Self {
        let storage_slots = mint_owner_controlled.0.to_storage_slots(
            BurnOwnerControlled::allowed_policy_proc_roots_slot(),
            BurnOwnerControlled::active_policy_proc_root_slot(),
        );

        todo!()
    }
}

This is just meant to illustrate the overall structure. Iiuc, the same applies to AuthControlled.

For the *PolicyAuthority, it might be fine to leave this duplicated since it's fairly little code.

[onurinanc]

Yes, but I do think that this deduplication makes the things more complicated. I'm not sure this is the ideal way to do it, I've tried to deduplicate but it doesn't seem good. I would prefer to address this again after we add RoleBasedAccessControlled for mint and burn policies. What do you think?

[PhilippGackstatter]

Nit: "No checks yet" sounds like we'll add some meaningful checks in the future, but this policy allows everything, so is this accurate? If we change it here, let's also change it in the mint policy one.

[PhilippGackstatter]

I didn't notice this in the last PR, but since this procedure is exec-uted, we should remove the pad comments because there will actually be meaningful values on the stack that we shouldn't drop.

Applies to all burn and mint policies.

[PhilippGackstatter]

Question: Why is the burn policy expected to return the asset if we require it to be identical? This means we could pass a copy of the asset to the policy instead and have it not return anything.

Also, loading the asset from the active note works, but incurs a lot of extra work (piping data from the advice provider through memory), and it would be easy to pass the asset instead, since this procedure is called from a trusted context.

So, ideally we can have this procedure be:

#! Executes active burn policy for the provided asset by dynamic execution.
#!
#! Inputs:  [ASSET_KEY, ASSET_VALUE]
#! Outputs: []
#!
#! Panics if:
#! - burn policy root is invalid.
#! - active burn policy validation fails.
#! - active burn policy mutates the asset key or value.
#!
#! Invocation: exec
@locals(4)
pub proc execute_burn_policy
    exec.get_burn_policy_root
    # => [BURN_POLICY_ROOT, ASSET_KEY, ASSET_VALUE]

    exec.assert_existing_policy_root
    # => [BURN_POLICY_ROOT, ASSET_KEY, ASSET_VALUE]

    exec.assert_allowed_policy_root
    # => [BURN_POLICY_ROOT, ASSET_KEY, ASSET_VALUE]

    loc_storew_le.BURN_POLICY_PROC_ROOT_PTR dropw
    # => [ASSET_KEY, ASSET_VALUE]

    locaddr.BURN_POLICY_PROC_ROOT_PTR
    # => [policy_root_ptr, ASSET_KEY, ASSET_VALUE]

    dynexec
    # => []
end

Note also that I've removed the pad comments, because this procedure is executed and should not drop any additional elements from the stack. We have 3-4 more procedures in this module and in the mint policy manager that have Invocation: exec but have pad comments. To avoid future mistakes due to these pad comments, it'd be good to remove these.

[onurinanc]

Hi @PhilippGackstatter, could you review this when you have a chance?

I’m wondering whether the deduplication related to thie comment: #2664 (comment) is actually better now, or if it ended up making things more complex. Right now, it feels like the storage slots don’t have a clear structure anymore.