Conversation
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left just a few questions/comments inline.
| #! This procedure does not verify the caller. Wrap with access control in the account component | ||
| #! composition if only privileged accounts should pause. |
There was a problem hiding this comment.
This will be changed in a follow-up PR, right? (i.e., we'll have owner-based or RBAC-based checks)
There was a problem hiding this comment.
I think this comment won't be changed but instead, owner-based and RBAC-based checks will include their own assertions using this procedure or with a policy manager if we also want to include dyncall here similar to blocklistable (transfer_controls), but I think we somehow need to treat both transfer_controls through blocking specific accounts, pausing the global accounts, and transfer amount restrictions as they all need to be included in the same callback.
But, I believe, for Pausable, we have decided to go with the Approach 2, or should we switch to Approach 3? #2241 (comment)
There was a problem hiding this comment.
I think I don't yet understand how we want to expose these procedures. Are pause and unpause supposed to be called by trusted code, i.e. exposed through other public account procedures? If so, we shouldn't export them from crates/miden-standards/asm/account_components/utils/pausable.masm because that would allow anyone to call these without authentication.
I assume we want to define something like:
pub proc pause_with_role
exec.active_note::get_sender
push.PAUSER_ROLE_SYMBOL
exec.::miden::standards::access::role_based_access_control::has_role
assert.err="sender does not have the pauser role"
exec.::miden::standards::utils::pausable::pause
end
The nice thing is that the pause impl itself is define in standards and can be extended with various auth mechanisms. The downside is that this procedure would have a MAST root specific to the RBAC implementation, and another implementation using ownable would have a different MAST root. So to pause a network account, we'd need to have distinct PAUSE_RBAC and PAUSE_OWNABLE network note types.
If we want stable MAST roots to enable a single PAUSE note to work for all impls, we probably need dyncalls as mentioned in approach C in #2241 (comment). The main question is where we want to have the complexity, in the pausable implementation (dynamic execution) or on the user side (clients need to handle multiple pause notes). It's probably better to bite the bullet and make it simpler for users, and it would work consistently with the other MINT and BURN note types we have.
I guess we'd need another PausePolicyManager, but since we're considering a unified TokenPolicyManager (#2821 (comment)), it likely makes sense to include the pause functionality in that manager, too.
There was a problem hiding this comment.
crates/miden-standards/asm/account_components/utils/pausable.masm because that would allow anyone to call these without authentication.
That's a good point, so I think pausable.masm should be a module rather than providing an account component for it (for now before pausable with Ownable2Step and RBAC), and removing it as an account component and adding the component as a test script makes sense.
Other than that, I think your point is to implement approach C as we also talk in this comment as a part of TokenPolicyManager #2821 (comment).
But, the main issue with merging MASM files specified in approach 2 in my comment here: #2821 (comment) and your comment here: #2821 (comment) that we might also need a TransferControlPolicyManager (Blocklistable) as a part of this PR, too: #2820 (comment). Or, there might be other modules needs to be included here, too, in the future.
So, I'm not actually sure if the dyncall is the correct approach for "transfer control" (blocklistable) or "pausable", I think we need to provide modules and user needs to implement its policies at some point. (I'm not sure what's the correct way right now but I'll be trying to come up with a better solution)
There was a problem hiding this comment.
Ah - makes sense! I should read some comments more carefully. So, the idea is that in a follow-up PR account_components/utils/pausable.masm would go away and instead we'll have something like OwnerPausable and RbacPausable components - right?
There was a problem hiding this comment.
One thing I just realized is that having different components for pausable access control would imply that we'd need different notes to invoke the right set of procedures on each. Not a big deal (especially if the number of such components is small) - but something to be aware of.
There was a problem hiding this comment.
I think we have bunch of PRs open but in case of the workflow is not clear as expected I've prepared how workflow would be:
Current Workflow for Stablecoin and Access Control:
- Extraction (Main PR): refactor: Extract Mint and Burn PolicyManager into their own component #2821
- RBAC (Main PR): feat(standards): add Role Based Access Control Account Component #2712
- Pausable: Pausable #2793
- Blocklistable (Account Freezing): feat(standards): add Blocklistable component for per-account freezing #2820
Core PRs:
- Extraction and RBAC are the foundational PRs that the rest of the work will build on:
- Extraction: Introduces
TokenPolicyManager(mint/burn combination). - RBAC: A new access control mechanism built on top of
Ownable2Step.
- Extraction: Introduces
Once these two are merged, they will serve as the foundation for everything that follows.
Plan After Merging the Foundation PRs
Pausable
- Merging the Pausable PR largely as-is, without using
dyncall. We'll extract the pausable logic out of the account components, and for testing purposes, implement this account component inside scripts.
Pausable follow-ups - implement the following as account components:
PausableOwnable2StepPausableRbac
Blocklistable
We are not merging this PR yet. Once the Extraction PR is merged, we'll rework its architecture as follows:
- Add transfer to
TokenPolicyManager. - Add the
on_before_asset_added_to_accountandon_before_asset_added_to_notecallbacks intoTransferPolicyManager. These callbacks will executeexecute_transfer_policy. - Introduce two owner-controlled transfer policies:
allow_alltransfer_if_not_blocked
The active policy will be invoked via dyncall inside execute_transfer_policy:
allow_all→ no-optransfer_if_not_blocked→ executesassert_not_in_blocklist
After Pausable is added:
- Extend the owner-controlled transfer policies described above:
transfer_if_not_paused→ executesassert_not_pausedtransfer_if_not_paused_and_not_blocked→ executes bothassert_not_in_blocklistandassert_not_paused
After RBAC is added:
- mint and burn → RBAC-controlled
- transfer → RBAC-controlled
- more Policies → RBAC-controlled
There was a problem hiding this comment.
All makes sense! Thank you for writing this up, @onurinanc!
| pub proc on_before_asset_added_to_account | ||
| exec.assert_not_paused | ||
| # => [ASSET_KEY, ASSET_VALUE, pad(8)] | ||
|
|
||
| dropw | ||
| # => [ASSET_VALUE, pad(12)] | ||
| end | ||
|
|
||
| #! Callback when a callbacks-enabled asset is added to an output note. | ||
| #! | ||
| #! Panics if this faucet account is paused. | ||
| #! | ||
| #! Inputs: [ASSET_KEY, ASSET_VALUE, note_idx, pad(7)] | ||
| #! Outputs: [ASSET_VALUE, pad(12)] | ||
| #! | ||
| #! Invocation: call | ||
| pub proc on_before_asset_added_to_note |
There was a problem hiding this comment.
Why do we need these callbacks here? I was imagining that they'd be defined in more specific contexts (e.g., in regulated stablecoin).
There was a problem hiding this comment.
Correct, I've added them initially for testing purposes, but now, I've added a MockPausableCallback component which includes these two callback procedures, and removed them here.
There was a problem hiding this comment.
@onurinanc FYI The callbacks are still present in the latest version.
There was a problem hiding this comment.
@PhilippGackstatter thank you for the comment, what I meant from the MockPausableCallback is that, instead of having on_before_asset_added_to_account and on_before_asset_added_to_note callbacks as a part of the pausable.masm component, I've created a mock component for testing purposes instead using pausable.masm by adding those 2 callbacks.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline - but overall, as you've outlined, I'd come back to this PR after we get the core PRs merged (i.e., the RBAC and mint/burn token policies).
I would maybe even come back to this after we've implemented transfer policies and merged fungible faucets into a single component. I think the overall structure would be much more clear then.
| pub use ::miden::standards::utils::pausable::is_paused | ||
| pub use ::miden::standards::utils::pausable::pause | ||
| pub use ::miden::standards::utils::pausable::unpause |
There was a problem hiding this comment.
I'm not sure pause and unpause should be included here. As @PhilippGackstatter mentioned in one of the comments, these are not meant to be used externally (i.e., they are to be invoked only by other components deployed within the same account) - and so, shouldn't be a part of the "external" interface.
| #! Invocation: call | ||
| pub proc pause |
There was a problem hiding this comment.
Related to the above comment, I think pause (and unpause) should be invokable via the exec instruction rather than the call instruction because they are meant only for internal use.
|
I think the current implementation of this PR is closer to the approach specified here: #2862 (comment) |
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Not a review yet, but I left one question inline about potentially using a different approach.
Also, "pausing" a faucet in my mind could imply two things:
- Pausing transfers of all assets issued by the faucet (only possible for assets that enable callbacks).
- Pausing minting of new assets.
I believe this PR addresses the first point but not the second. Should we cover both points under the same "pause" functionality? Or should we have distinct procedures for pause_transfers, pause_minting, and pause_transfers_and_minting?
Lastly, it may be good to follow the pattern described in #2862 (comment) - though, if we are using transfer/mint policies to control pausing/unpausing - there wouldn't be a distinct "pauser" role.
| /// Active policy = [`BasicPausable::root`]. Resolves into a [`BasicPausable`] component that | ||
| /// starts unpaused; to seed an initial paused state, install [`BasicPausable`] explicitly | ||
| /// via [`BasicPausable::paused`] and select the policy via [`TransferPolicy::Custom`] with | ||
| /// [`BasicPausable::root`]. | ||
| Pausable, | ||
| /// Active policy = [`PausableBlocklist::root`]. Resolves into a [`PausableBlocklist`] | ||
| /// component that starts unpaused and with no initially blocked accounts; to seed either | ||
| /// state, install [`PausableBlocklist`] explicitly via | ||
| /// [`PausableBlocklist::with_initial_pause_state`] / | ||
| /// [`PausableBlocklist::with_initial_blocked_accounts`] and select the policy via | ||
| /// [`TransferPolicy::Custom`] with [`PausableBlocklist::root`]. | ||
| PausableBlocklist, |
There was a problem hiding this comment.
Do we actually need these variants? IIUC, the functionality we want is to have the ability to pause all asset transfers for a given faucet. If so, this could probably be achieved with something like DenyAll policy that will error out on any asset transfer.
If the owner of the faucet would want to pause all transfers, they would be able to set the active transfer policies to DenyAll, and then, if they want to unpause, they set the active transfer policy to something else.
So, it seems to me that adding something like DenyAll transfer policy should be sufficient - but maybe I'm missing something?
There was a problem hiding this comment.
If the owner of the faucet would want to pause all transfers, they would be able to set the active transfer policies to DenyAll, and then, if they want to unpause, they set the active transfer policy to something else.
Do you mean if they want to switch to another policy component in this part: "if they want to unpause"?
The main reason why we have this variants is an account should be able to choose which policy checks they want to include in their code.
For example,
- if a user wants to use all checks they can add the
DenyAllpolicy check component, which uses pausable, blocklist, allowlist. - if a user doesn't want to include allowlist in their account, they can just add
PausableBlocklisttransfer policy check component into their account, which doesn't include allowlist checks but include pausable and blocklist checks. - if a user wants to include both
DenyAllandPausableBlocklistthey can switch to one of them later. byset_{send/receive}_policy
|
Thank you @bobbinth for the comments.
I think we should also include burning assets to this list as third item. I was thinking about handling 2 and 3 by adding new mint/burn policies but it's also fine to me to add this features in this PR.
I believe pausing is an emergency situation so it should be just one For the concept of stablecoin, cross-chain bridging, NFTs, I consider For some defi protocols such as staking and lending, pause might be needed for specific procedures such as in addition to mint and transfer, borrow might be needed to be paused (as global pause still satisfies this) but withdraw might be unpaused, for example. |
Agreed.
Agreed here as well. So, the I've also been thinking about how to implement this in the "least intrusive" way. For mint and burn, this should be pretty easy. Specifically, we should be able to avoid introducing any new policies and handle the check at the
For transfers, we can't use the same pattern because use use the callback slots to directly store the policy procedure roots. But I do wonder if instead of introducing new policy variants (e.g., "pausable blocklist"), we could add an extra layer of indirection and make it work more similar to the mint/burn policies. The approach could be:
This would require two extra storage slots, and an extra call indirection - but maybe that's worth it. |
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I reviewed mostly non-test code and left some comments inline. In the interest of getting this merged sooner, I think we should only address the simple comments here, and leave the bigger ones to a follow-up PR.
The follow-ups would include:
- Using
active_account::has_storage_slotprocedure to figure out if theis_pausedstorage slot is installed in an account. This procedure first needs to be added to the transaction kernel. - Adding a level of indirection to the transfer policy handlers so that we could control them from the
TokenPolicyManager(similar to how we control mint/burn policies).
| #! If the `IS_PAUSED_SLOT` is not installed on the account, `active_account:: get_item` | ||
| #! returns the zero word, which is treated as "unpaused" — so the assertion is a no-op | ||
| #! for accounts that did not install the Pausable component. This is the canonical way for | ||
| #! cross-cutting consumers (TokenPolicyManager dispatch, asset callbacks, metadata setters) to | ||
| #! gate their logic on pause state without making Pausable a hard dependency. |
There was a problem hiding this comment.
Is the behavior described here correct? The docs for active_account::get_item say that it should panic if the requested storage slot does not exist. cc @PhilippGackstatter
If active_account::get_item does not panic on non-existent storage slot, that's a bug and we should fix it (let's open an issue for this).
If it does panic, the we should update the logic here, but AFACT, we actually don't have something like active_account::has_storage_slot. I think we should add it and then based this procedure on that. But I would do it in a separate PR.
There was a problem hiding this comment.
The documented behavior should be correct, get_item panics on unknown slots. test_account_get_item_fails_on_unknown_slot tests that.
The idea is that, because assert_not_paused is used so pervasively, we don't want to force every faucet to explicitly set that slot, if they don't use pause functionality? I guess that's fine, though being explicit about it also has its own value. I'd lean towards being explicit, but not a strong opinion.
The has_storage_slot functionality may be interesting independently, though I'd add it when we need it.
There was a problem hiding this comment.
I think this is more about implied dependencies. Without the extra check, TokenPolicyManager will require Pausable as a dependency. So, anyone deploying a faucet would need to also add the Pausable component to their account, even if they don't need this functionality.
| let mut slots: Vec<StorageSlot> = Vec::new(); | ||
| slots.push(self.token_config_slot_value()); | ||
| slots.extend(self.metadata.into_storage_slots()); | ||
| slots.push(crate::account::pausable::PausableStorage::default().into_slot()); |
There was a problem hiding this comment.
Why do we need to do this via the FungibleFaucet component rather than via the Pausable component?
| #! | ||
| #! Invocation: call | ||
| pub proc check_policy | ||
| exec.pausable::assert_not_paused |
There was a problem hiding this comment.
This is fine for now, but I still think we should consider the approach I've described in #2793 (comment). The main reason is that with the current approach, developers will need to make sure add this assertion to all future policies, and that may be something easy to miss. Handling it at a PolicyManager level removes this potential footgun.
But let's not do this in this PR and instead create an issue.
| pub struct PausableStorage { | ||
| initial_state: bool, | ||
| } |
There was a problem hiding this comment.
I would rename initial_state to just state as we should be able to instantiate PausableStorage from existing accounts too.
| /// Creates a [`PausableStorage`] starting unpaused. | ||
| pub const fn new() -> Self { | ||
| Self { initial_state: false } | ||
| } | ||
|
|
||
| /// Creates a [`PausableStorage`] with the given initial pause state. | ||
| pub const fn with_initial_state(initial_state: bool) -> Self { | ||
| Self { initial_state } | ||
| } |
There was a problem hiding this comment.
I would get rid of the current new() constructor (the Default implementation does the same thing), and would rename with_initial_state() to new().
| pub fn build_word(&self) -> Word { | ||
| if self.initial_state { | ||
| Word::from([1u32, 0, 0, 0]) | ||
| } else { | ||
| Word::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: I would rename this to to_word().
| pub struct Pausable { | ||
| initial_state: bool, | ||
| } |
There was a problem hiding this comment.
Same comment as above: I'd rename initial_state into state.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me! Left a few comments, nothing blocking.
I agree with the follow-up of moving the assert_not_paused into the token policy manager. Pause functionality seems to be quite fundamental, so making the manager pause-aware makes sense to me.
There was a problem hiding this comment.
I think utils is a category we should avoid because it is so generic. Even if it doesn't fit perfectly, I'd rather move this module into access. Pausing is a form of global access control, so it fits at least a bit.
| #! If the `IS_PAUSED_SLOT` is not installed on the account, `active_account:: get_item` | ||
| #! returns the zero word, which is treated as "unpaused" — so the assertion is a no-op | ||
| #! for accounts that did not install the Pausable component. This is the canonical way for | ||
| #! cross-cutting consumers (TokenPolicyManager dispatch, asset callbacks, metadata setters) to | ||
| #! gate their logic on pause state without making Pausable a hard dependency. |
There was a problem hiding this comment.
The documented behavior should be correct, get_item panics on unknown slots. test_account_get_item_fails_on_unknown_slot tests that.
The idea is that, because assert_not_paused is used so pervasively, we don't want to force every faucet to explicitly set that slot, if they don't use pause functionality? I guess that's fine, though being explicit about it also has its own value. I'd lean towards being explicit, but not a strong opinion.
The has_storage_slot functionality may be interesting independently, though I'd add it when we need it.
| /// - [`Self::is_paused_slot()`]: single word; the zero word means unpaused, `[1, 0, 0, 0]` means | ||
| /// paused. Any non-zero word is interpreted as paused by the MASM helpers. | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub struct PausableStorage { |
There was a problem hiding this comment.
Iiuc, we only use this in Pausable, so why is this a separate type? I have the same question about BasicBlocklist and BlocklistStorage.
If it must be a separate type, I'd redefine Pausable(PausableStorage), unless that creates issues.
| // ================================================================================================ | ||
|
|
||
| #[tokio::test] | ||
| async fn pausable_manager_pause_succeeds_when_owner_signs() -> anyhow::Result<()> { |
There was a problem hiding this comment.
| async fn pausable_manager_pause_succeeds_when_owner_signs() -> anyhow::Result<()> { | |
| async fn pausable_manager_pause_succeeds_when_sender_is_owner() -> anyhow::Result<()> { |
I don't think the owner signs here?
| let mut rng = | ||
| RandomCoin::new(Word::from([1u32, 2, 3, 4].map(|v| Felt::new_unchecked(v as u64)))); |
There was a problem hiding this comment.
| let mut rng = | |
| RandomCoin::new(Word::from([1u32, 2, 3, 4].map(|v| Felt::new_unchecked(v as u64)))); | |
| let mut rng = RandomCoin::new(Word::from([1u32, 2, 3, 4])); |
| # ERRORS | ||
| # ================================================================================================ | ||
|
|
||
| const ERR_PAUSABLE_ENFORCED_PAUSE = "the contract is paused" |
There was a problem hiding this comment.
Nit: I'd rename this to ERR_PAUSABLE_IS_PAUSED.
| let seed: [u64; 4] = rand::random(); | ||
| let mut rng = RandomCoin::new(Word::from(seed.map(Felt::new_unchecked))); |
There was a problem hiding this comment.
| let seed: [u64; 4] = rand::random(); | |
| let mut rng = RandomCoin::new(Word::from(seed.map(Felt::new_unchecked))); | |
| let seed: [u32; 4] = rand::random(); | |
| let mut rng = RandomCoin::new(Word::from(seed)); |
Closes issue including the design discussion provided here: #2241
This PR adds a Pausable component without Access Control mechanism included as we split this work into small PRs.
In the next PRs, we'll integrate the following features:
Related Issues: