refactor: merge AuthMethod into AccessControl#2944
Conversation
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Not a review yet, but I left some comments inline. For now, more questions/thoughts than concrete suggestions.
| /// Yields the [`AccountComponent`]s implementing this access control configuration, in the | ||
| /// order they must be installed on the account. The matching [`Authority`] component is | ||
| /// always included. |
There was a problem hiding this comment.
was this comment removed on purpose?
| pub enum AccountAuthScheme { | ||
| NoAuth, | ||
| SingleSig, | ||
| SingleSigAcl, | ||
| NetworkAccount, | ||
| Multisig, | ||
| GuardedMultisig, | ||
| MultisigSmart, | ||
| Custom, | ||
| } | ||
|
|
||
| /// A typed wrapper around an authentication [`AccountComponent`]. | ||
| pub struct AccountAuthComponent { | ||
| inner: AccountComponent, | ||
| scheme: AccountAuthScheme, | ||
| } |
There was a problem hiding this comment.
Taking a step back, it seems like these helpers for faucet creation (create_user_fungible_faucet, create_network_fungible_faucet) inucr a high amount of additional code we need for questionable gain (client uses it in tests only, node doesn't use it at all). I'd love to get rid of them, but maybe that's too drastic. At the very least, I think we should simplify these. In my opinion, it's totally fine to be opinionated here - it's always possible to fall back to the AccountBuilder and to add more functions rather than to do more with the same function.
I think the main complication comes from us trying to support various auth components in the creation of the faucets. Right now we support NoAuth for network accounts and custom account components for each, but really, the meaningful configurations we can support are AuthSinglesigAcl for users faucets as well as AuthNetworkAccount for network accounts. For custom components, we cannot meaningfully validate anything in these helpers anyway, so there is no gain from supporting these. Users might as well use the AccountBuilder directly. I don't think we should allow NoAuth anyway, because it is dangerous for network accounts. That one might be convenient for tests, but we can introduce a separate testing-gated helper for this, if needed.
So, I think we can just require the concrete component to be passed:
pub fn create_user_fungible_faucet(
init_seed: [u8; 32],
faucet: FungibleFaucet,
auth_component: AuthSingleSigAcl,
token_policy_manager: TokenPolicyManager,
account_type: AccountType,
) -> Result<Account, FungibleFaucetError> {
todo!()
}
pub fn create_network_fungible_faucet(
init_seed: [u8; 32],
faucet: FungibleFaucet,
access_control: AccessControl,
auth_component: AuthNetworkAccount,
token_policy_manager: TokenPolicyManager,
account_type: AccountType,
) -> Result<Account, FungibleFaucetError> {
todo!()
}This means no need for AccountAuthScheme (*) and AccountAuthComponent, no need to have extensive docs describing what auth components are supported and which are rejected, no need to test that certain components are rejected, and an API that tells you at compile time whether the configuration is valid rather than at runtime.
If we want to introduce additonal helpers for, say, a multisig-user faucet, we can add this as a separate function.
(*) Right now the AccountComponentInterface still soft-requires us to have this auth scheme enumeration, but we should get rid of this very soon (#2621), so I'd keep it in the most minimal form possible that makes the existing code work.
|
Applied the changes considering the possible direction commented here: #2944 (comment) |
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks great! I think this is a much-needed relief on complexity.
Left a few nits and suggestions, nothing blocking.
| pub fn user_faucet_single_sig_acl( | ||
| pub_key: miden_protocol::account::auth::PublicKeyCommitment, | ||
| scheme: miden_protocol::account::auth::AuthScheme, | ||
| ) -> Result<AuthSingleSigAcl, FungibleFaucetError> { |
There was a problem hiding this comment.
Seems to be a test-only helper, so I'd move it behind testing.
| let allowlist: BTreeSet<NoteScriptRoot> = | ||
| [NoteScriptRoot::from_raw(Word::new([Felt::ONE; 4]))].into_iter().collect(); |
There was a problem hiding this comment.
| let allowlist: BTreeSet<NoteScriptRoot> = | |
| [NoteScriptRoot::from_raw(Word::new([Felt::ONE; 4]))].into_iter().collect(); | |
| let allowlist: BTreeSet<NoteScriptRoot> = | |
| [NoteScriptRoot::from_array([0; 4])].into_iter().collect(); |
Nit: We have a nice helper for test construction.
| #[derive(Debug, Error)] | ||
| pub enum BasicWalletError { | ||
| #[error("unsupported authentication method: {0}")] | ||
| UnsupportedAuthMethod(String), | ||
| #[error("account creation failed")] | ||
| AccountError(#[source] AccountError), | ||
| } |
There was a problem hiding this comment.
nit: I'm not sure this error is meaningful anymore. We might as well return AccountError from create_basic_wallet.
| fn create_new_user_fungible_faucet( | ||
| &mut self, | ||
| auth_method: Auth, | ||
| faucet: FungibleFaucet, | ||
| account_type: AccountType, | ||
| access_control: AccessControl, | ||
| token_policy_manager: TokenPolicyManager, | ||
| ) -> anyhow::Result<Account> { | ||
| let account_builder = AccountBuilder::new(self.rng.random()) | ||
| .account_type(account_type) | ||
| .with_component(faucet) | ||
| .with_components(access_control) | ||
| .with_component(Authority::AuthControlled) | ||
| .with_components(token_policy_manager); | ||
|
|
||
| self.add_account_from_builder(auth_method, account_builder, AccountState::New) |
There was a problem hiding this comment.
Seems like we could inline this logic into create_new_faucet now.
| /// Internal helper: adds an existing user-account fungible faucet (AuthControlled). | ||
| fn add_existing_user_fungible_faucet( |
There was a problem hiding this comment.
Same with this, should we inline this into add_existing_basic_faucet?
| .into_iter() | ||
| .chain([MintNote::script_root(), BurnNote::script_root()]) | ||
| .collect(); | ||
| let allowed_script_roots: alloc::collections::BTreeSet<NoteScriptRoot> = |
There was a problem hiding this comment.
| let allowed_script_roots: alloc::collections::BTreeSet<NoteScriptRoot> = | |
| let allowed_script_roots: BTreeSet<NoteScriptRoot> = |
nit
| - Derive `Hash` implementation for `StorageMapKey` and `StorageMapKeyHash` to allow using those values as keys in containers ([#2843](https://github.com/0xMiden/protocol/issues/2843)). | ||
| - [BREAKING] Removed `AuthMethod` enum and `AccessControl::AuthControlled` variant. Faucet factories now take concrete auth-component types so invalid configurations are rejected at compile time. ([#2944](https://github.com/0xMiden/protocol/pull/2944)). | ||
| - [BREAKING] Split `create_fungible_faucet` into `create_user_fungible_faucet` (takes `AuthSingleSigAcl`, installs `Authority::AuthControlled` directly) and `create_network_fungible_faucet` (takes `AuthNetworkAccount` and `AccessControl::Ownable2Step | Rbac`). For any other auth scheme, use `AccountBuilder` directly. A new `user_faucet_single_sig_acl` helper builds an `AuthSingleSigAcl` with the complete authority-gated setter trigger list. ([#2944](https://github.com/0xMiden/protocol/pull/2944)). | ||
| - [BREAKING] `create_basic_wallet` now takes `AuthSingleSig` directly instead of a wrapped auth type. For multisig wallets, use `AccountBuilder` directly. ([#2944](https://github.com/0xMiden/protocol/pull/2944)). | ||
| - [BREAKING] Removed `AccountInterface::auth()` method (returning `Vec<AccountAuthScheme>`). Auth components are now discovered via `AccountInterface::auth_components()`, which iterates `AccountComponentInterface` variants flagged by `is_auth_component()`. ([#2944](https://github.com/0xMiden/protocol/pull/2944)). | ||
| - Optimized `B2AGG` processing with selective load/save of Local Exit Tree frontier entries, halving frontier storage map syscalls ([#2752](https://github.com/0xMiden/protocol/pull/2752)). | ||
| - [BREAKING] Removed `AccountType` ([#2939](https://github.com/0xMiden/protocol/pull/2939)). |
There was a problem hiding this comment.
This might be the result of some auto union-merge, could you double check if there are duplicate entries now?
In any case, this PR's entry should go into the 0.16 section.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few comments/questions inline.
Also, role assignment will be done in a different PR, right? Do we already have an issue for this?
| /// Caller passes a fully-configured [`AuthSingleSigAcl`] — its trigger procedure list must | ||
| /// cover every authority-gated setter on the faucet (`mint_and_send`, the metadata setters, | ||
| /// the policy setters, and `pause` / `unpause`), otherwise those procedures become | ||
| /// permissionless under [`Authority::AuthControlled`]. | ||
| pub fn create_user_fungible_faucet( |
There was a problem hiding this comment.
Just to clarify: the caller is expected to configure the ACL auth component with the roots of all procedures, right? Would this create friction for the user?
There was a problem hiding this comment.
Looks good, but I have a design question specifically about the faucet builder Rust functions.
The two faucet builders gate the faucet procedures differently:
- for
create_network_fungible_faucetyou passaccess_control(the owner/role), and the admin procedures (the ones that change the faucet's settings, likeset_max_supplyorpause) are all tied to a specific role or the owner. - for
create_user_fungible_faucetyou manually create a list (auth_trigger_procedures) of those same admin procedures, and any procedure you forget to add is callable by anyone.
create_network_fungible_faucet is safe by default: you pass access_control (the owner/role) and every authority-gated setter is gated to that owner in MASM, so the gating of procedures is complete when you use create_network_fungible_faucet.
However, create_user_fungible_faucet is the opposite: with AuthSingleSigAcl you specify the procedures which can only be called by the owner; any procedure root you forget to put in auth_trigger_procedures are callable by anyone. I think this should be reversed, specify the procedures which can be callable by anyone, every other procedure is onlyOwner by default.
The safer faucet builder functions (user_faucet_single_sig_acl / all_authority_gated_setter_roots) are behind the testing feature, so production users are forced to build the list of owner-only procedures manually.
I think this is a bit difficult to reason about and we should simplify this. I think this is how it used to be with create_fungible_faucet, right?
My idea would be to make the user path safe by default, users specify which exact procedures are "public": every authority setter is owner-only by default, and you list only the ones you want left open (usually none).
Something like this:
pub enum FaucetAuthority {
UserControlled {
pub_key: PublicKeyCommitment,
scheme: AuthScheme,
account_type: AccountType,
permissionless: Vec<AccountProcedureRoot>, // opt-out, usually empty
},
NetworkControlled { access_control: AccessControl },
}
pub fn create_fungible_faucet(
init_seed: [u8; 32],
faucet: FungibleFaucet,
authority: FaucetAuthority,
token_policy_manager: TokenPolicyManager,
) -> Result<Account, FungibleFaucetError>
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few comments inline - but I think this is largely good to go.
We do need to rebase this on the latest next though.
| let note_allowlist = [MintNote::script_root(), BurnNote::script_root()].into_iter().collect(); | ||
| let auth_component = AuthNetworkAccount::with_allowed_notes(note_allowlist) | ||
| .expect("MintNote + BurnNote allowlist is non-empty"); |
There was a problem hiding this comment.
Not for this PR, but in the future, we'll need to:
- Add script roots of config notes to the note allowlist.
- Once feat: canonical expiration transaction script in miden-standards #3051 is merged, we'll need to add a tx script allow list with the expiration script root.
| mod auth_method; | ||
| pub use auth_method::AuthMethod; |
There was a problem hiding this comment.
I believe after this change the src/auth_method.rs file is no longer needed and can be removed.
| /// cover every authority-gated setter on the faucet (`mint_and_send`, the metadata setters, | ||
| /// the policy setters, and `pause` / `unpause`), otherwise those procedures become | ||
| /// permissionless under [`Authority::AuthControlled`]. | ||
| pub fn create_user_fungible_faucet( |
There was a problem hiding this comment.
I do agree with @partylikeits1983 that using this function correctly is not easy. I would still keep it separate though. Ideally, we'd be able to construct the auth component inside of this function.
One option could be to bring back the AuthMethod enum - but simplify it to be something like:
pub enum AuthMethod {
SingleSig {
approver: (PublicKeyCommitment, AuthScheme),
},
Multisig {
threshold: u32,
approvers: Vec<(PublicKeyCommitment, AuthScheme)>,
},
}And then construct the relevant components based on this enum.
Basically, AuthMethod would become very narrow and intended to be used only with this function.
In the interest of time, I'm also fine figuring out the right approach here in a follow-up PR.
There was a problem hiding this comment.
I think this would work, if we want to support multisigs as well. As mentioned on Slack, I think keeping the two functions for building a network faucet with access control and building a user faucet with authentication separate is an overall clearer API.
One could also argue that we could have create_singlesig_user_fungible_faucet and create_multisig_user_fungible_faucet because I'm not sure whether the AuthMethod abstraction is actually needed anywhere?
I think after #3065 we'd probably want to pass something like permissionless: Vec<AccountProcedureRoot> to the singlesig function as @partylikeits1983 suggested. Though, given that this function knows exactly what it's building, I think the only function that would potentially end up in the permissionless list is the burn_and_receive root, and so we could narrow this down to something like is_burn_permissionless: bool (though ideally replacing the bool with a two variant enum).
The underlying ACL approach will be changed to what you described in #3065. We'll then need to update |
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good! Didn't mean to hold up this PR with my comments above, I think we can think about these things in a separate issue and decide what to do regarding a followup PR
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline - once these are addressed, we should be good to merge.
@onurinanc - could you also create an issue with follow-ups that are coming out of this PR?
| pub fn auth_components(&self) -> impl Iterator<Item = &AccountComponentInterface> { | ||
| self.components.iter().filter(|c| c.is_auth_component()) | ||
| } |
There was a problem hiding this comment.
An account should contain only one auth component - so, this should probably return a single component interface.
| pub fn new(account_id: AccountId, components: Vec<AccountComponentInterface>) -> Self { | ||
| Self { account_id, components } |
There was a problem hiding this comment.
Related to the above comment, we should probably enforce that the passed in vector of account components contains exactly one auth component.
Closes: #2930
Fixes: #2943
Tasks:
build_auth_componentmethod added in Fix: AuthControlled faucet leaves Authority setters unauthenticated #2958create_fungible_faucetascreate_network_fungible_faucetandcreate_user_fungible_faucetspecified here: refactor: mergeAuthMethodintoAccessControl#2944 (comment)