refactor: merge `AuthMethod` into `AccessControl` by onurinanc · Pull Request #2944 · 0xMiden/protocol

onurinanc · 2026-05-18T14:44:55Z

Closes: #2930
Fixes: #2943

Tasks:

Consider removing build_auth_component method added in Fix: AuthControlled faucet leaves Authority setters unauthenticated #2958
Consider separating create_fungible_faucet as create_network_fungible_faucet and create_user_fungible_faucet specified here: refactor: merge AuthMethod into AccessControl #2944 (comment)

bobbinth

Thank you! Not a review yet, but I left some comments inline. For now, more questions/thoughts than concrete suggestions.

bobbinth · 2026-05-20T07:58:00Z

+/// Rejects [`AuthMethod::Multisig`] / [`AuthMethod::Unknown`] for all variants (faucets do
+/// not support Multisig today), and rejects [`AuthMethod::NoAuth`] specifically under
+/// [`AccessControl::AuthControlled`] because it would leave authority-gated setters
+/// permissionless.


There is no fundamental reason why faucets shouldn't support multisig-based auth. Let's create an issue for this - though, it'll probably be a fairly low priority.

bobbinth · 2026-05-20T09:08:19Z

@@ -59,29 +86,51 @@ pub enum AccessControl {
    Rbac {
        owner: AccountId,
        authority_role: Option<RoleSymbol>,
+        auth: AuthMethod,
    },


Adding AuthMethod to Ownable2Step and Rbac feels a bit off because currently the only "legal" value for these variants is AuthMethod::NetworkAccount. I think the fault mostly lies in the AuthMethod enum which is kind of in between an auth component and a pure enum.

It would be a bigger refactoring, but I think we should get rid of AuthMethod enum altogether and replace it with something like AccountAuthComponent struct which would be a wrapper over AccountComponent with some convenience constructors.

Adding AuthMethod to Ownable2Step and Rbac feels a bit off
I agree on this. Additionally, adding and AuthMethod to AccessControl doesn't sound well either. AuthControlled should be an account component similar to Ownable2Step and Rbac and having an AccountAuthComponent would be a good variant instead of combining AuthMethod with AuthControlled.

For this refactoring, do we also need #2621 this to be merged? similar to below comment?

Other than that, if we decide to remove AuthMethod and replace it with AccountAuthComponent, I think separating this PR into two:

The first one is as described above: Consider merging AuthMethod into AccessControl #2930

The second on is resolving this issue: Bug: AuthControlled faucet leaves Authority setters unauthenticated #2943

Yes, let's tackle #2943 separately (which I see you've already done) - and then we can come back here and do a more comprehensive refactoring.

bobbinth · 2026-05-20T09:12:01Z

 ///
 /// The faucet itself, including all token metadata, is provided in the `faucet` parameter (see
 /// [`FungibleFaucet::builder`]).
 pub fn create_fungible_faucet(


I wonder if we are trying to do too much with this single function. Maybe it is worth splitting it up with a couple functions - something like:

pub fn create_network_fungible_faucet( init_seed: [u8; 32], faucet: FungibleFaucet, access_control: AccessControl, // this would contain only Ownable2Step and Rbac token_policy_manager: TokenPolicyManager, storage_mode: AccountStorageMode, ) -> Result<Account, FungibleFaucetError> { ... } pub fn create_user_fungible_faucet( init_seed: [u8; 32], faucet: FungibleFaucet, auth_method: AuthMethod, // this would not contain NetworkAccount token_policy_manager: TokenPolicyManager, storage_mode: AccountStorageMode, ) -> Result<Account, FungibleFaucetError> { ... }

But again, this would be a bigger refactoring along the lines of #2621.

This approach seems to me better in that way we can separate AccountAuthComponent and AccessControl. We might also want to rename the AccessControl enum to NetworkAccessControl and doesn't include AccountAuthComponent as a variant in the enum.

mmagician

It looks like this PR contains most of the changes from #2958

mmagician · 2026-05-22T07:51:15Z

-    /// 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.


was this comment removed on purpose?

PhilippGackstatter · 2026-06-02T09:15:09Z

+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,
+}


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.

…th-controlled

onurinanc · 2026-06-03T14:50:26Z

Applied the changes considering the possible direction commented here: #2944 (comment)

PhilippGackstatter

Looks great! I think this is a much-needed relief on complexity.

Left a few nits and suggestions, nothing blocking.

PhilippGackstatter · 2026-06-03T15:09:22Z

+pub fn user_faucet_single_sig_acl(
+    pub_key: miden_protocol::account::auth::PublicKeyCommitment,
+    scheme: miden_protocol::account::auth::AuthScheme,
+) -> Result<AuthSingleSigAcl, FungibleFaucetError> {


Seems to be a test-only helper, so I'd move it behind testing.

PhilippGackstatter · 2026-06-03T15:13:44Z

+    let allowlist: BTreeSet<NoteScriptRoot> =
+        [NoteScriptRoot::from_raw(Word::new([Felt::ONE; 4]))].into_iter().collect();


Suggested change

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.

PhilippGackstatter · 2026-06-03T15:16:49Z

 #[derive(Debug, Error)]
 pub enum BasicWalletError {
-    #[error("unsupported authentication method: {0}")]
-    UnsupportedAuthMethod(String),
    #[error("account creation failed")]
    AccountError(#[source] AccountError),
 }


nit: I'm not sure this error is meaningful anymore. We might as well return AccountError from create_basic_wallet.

PhilippGackstatter · 2026-06-03T15:20:26Z

+    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)


Seems like we could inline this logic into create_new_faucet now.

PhilippGackstatter · 2026-06-03T15:20:57Z

+    /// Internal helper: adds an existing user-account fungible faucet (AuthControlled).
+    fn add_existing_user_fungible_faucet(


Same with this, should we inline this into add_existing_basic_faucet?

PhilippGackstatter · 2026-06-03T15:21:38Z

-            .into_iter()
-            .chain([MintNote::script_root(), BurnNote::script_root()])
-            .collect();
+        let allowed_script_roots: alloc::collections::BTreeSet<NoteScriptRoot> =


Suggested change

let allowed_script_roots: alloc::collections::BTreeSet<NoteScriptRoot> =

let allowed_script_roots: BTreeSet<NoteScriptRoot> =

nit

PhilippGackstatter · 2026-06-03T15:23:16Z

+- 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)).


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

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?

bobbinth · 2026-06-05T02:18:56Z

 ### Fixes

- Fixed auth components to use initial storage state for authentication ([#2677](https://github.com/0xMiden/protocol/issues/2677)).
+- Fixed `LocalTransactionProver` accumulating `MastForest` entries across `prove()` calls, causing `capacity_overflow` panics in WASM environments where linear memory fragmentation prevents subsequent allocations ([#2918](https://github.com/0xMiden/protocol/pull/2918)).


I think this and a few other unrelated changes snuck into the changelog somehow.

bobbinth · 2026-06-05T02:28:36Z

 pub fn create_basic_wallet(
    init_seed: [u8; 32],
-    auth_method: AuthMethod,
-    account_storage_mode: AccountType,
-) -> Result<Account, BasicWalletError> {
-    let auth_component: AccountComponent = match auth_method {
-        AuthMethod::SingleSig { approver: (pub_key, auth_scheme) } => {
-            AuthSingleSig::new(pub_key, auth_scheme).into()
-        },
-        AuthMethod::Multisig { threshold, approvers } => {
-            let config = AuthMultisigConfig::new(approvers, threshold)
-                .and_then(|cfg| {
-                    cfg.with_proc_thresholds(vec![(BasicWallet::receive_asset_root(), 1)])
-                })
-                .map_err(BasicWalletError::AccountError)?;
-            AuthMultisig::new(config).map_err(BasicWalletError::AccountError)?.into()
-        },
-        AuthMethod::NoAuth => {
-            return Err(BasicWalletError::UnsupportedAuthMethod(
-                "basic wallets cannot be created with NoAuth authentication method".into(),
-            ));
-        },
-        AuthMethod::NetworkAccount { .. } => {
-            return Err(BasicWalletError::UnsupportedAuthMethod(
-                "basic wallets cannot be created with NetworkAccount authentication method".into(),
-            ));
-        },
-        AuthMethod::Unknown => {
-            return Err(BasicWalletError::UnsupportedAuthMethod(
-                "basic wallets cannot be created with Unknown authentication method".into(),
-            ));
-        },
-    };
-
-    let account = AccountBuilder::new(init_seed)
-        .account_type(account_storage_mode)
+    auth_component: AuthSingleSig,
+    account_type: AccountType,
+) -> Result<Account, AccountError> {


Previously, we could use this to create multisig wallets as well. Should we add another helper function to support this functionality? For example, something like create_multisig_wallet().

Another thing, maybe we could also create a helper for creating guardian-supported wallets - e.g., create_guarded_wallet(). But maybe I'm going too far.

bobbinth · 2026-06-05T03:27:59Z

+/// 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(


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?

bobbinth · 2026-06-05T03:32:25Z

+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> {


A couple of comments on this:

We don't need to pass account_type here as network accounts must be public.

Why do we need to pass AuthNetworkAccount component? Could we not create/configure it automatically in this function?

Why do we need to pass AuthNetworkAccount component? Could we not create/configure it automatically in this function?

I think if we did that, we'd have to take two parameters (note allowlist, tx script allowlist) as input rather than one (AuthNetworkAccount), so the current approach seems a bit cleaner to me.

But then the caller needs to know the internal details of this function - no? Specifically, they need to know that config notes for the Pausable component need to be added to the note allowlist.

More generally though, can't this procedure build both the note script allowlist and tx script allowlist? If we can't do it here, there may not be much value in having this function - i.e., using the builder directly would result in a more readable/maintainable code.

My understanding is that we are OK with being pretty opinionated with this function (we are providing a quick way to construct a faucet w/o using the builder directly) - and so, we can make assumptions about notes/scripts allowed for this account.

Ah, that's true. Agreed, we should be able to configure the auth component internally and being opinionated is fine here.

bobbinth · 2026-06-05T03:34:08Z

        .with_component(faucet)
        .with_components(access_control)
        .with_components(token_policy_manager)
        .with_component(PausableManager)


Is adding PausableManager component enough? Should we not also add Pausable component?

onurinanc added 3 commits May 18, 2026 16:43

merge AuthMethod into AccessControl

2a93cf1

changelog

8b2527b

fix documentation

039ced3

bobbinth reviewed May 20, 2026

View reviewed changes

merge next

2a182da

onurinanc mentioned this pull request May 21, 2026

Fix: AuthControlled faucet leaves Authority setters unauthenticated #2958

Merged

mmagician reviewed May 22, 2026

View reviewed changes

remove AuthEnum and add AccountAuthComponent

7259d7b

mmagician requested a review from Fumuran May 28, 2026 08:12

onurinanc added 4 commits June 1, 2026 16:03

merge next

6701ddd

remove unnecessary comments

c55cebd

lint

a8fc71e

fix lint

6c7a405

onurinanc requested review from PhilippGackstatter and bobbinth June 1, 2026 14:32

PhilippGackstatter reviewed Jun 2, 2026

View reviewed changes

onurinanc added 4 commits June 3, 2026 15:11

Merge branch 'next' into fix-auth-controlled

db1cbf7

remove AccountAuthScheme and AccountAuthComponent

ab964d5

Merge remote-tracking branch 'origin/next' into fix-auth-controlled

cc52fdb

Merge remote-tracking branch 'origin/fix-auth-controlled' into fix-au…

928f464

…th-controlled

onurinanc requested review from PhilippGackstatter and mmagician June 3, 2026 14:50

PhilippGackstatter approved these changes Jun 3, 2026

View reviewed changes

onurinanc added 3 commits June 4, 2026 12:15

apply suggestions

08471ba

Merge remote-tracking branch 'origin/next' into fix-auth-controlled

f1f961a

lint

3b9fe38

bobbinth reviewed Jun 5, 2026

View reviewed changes

onurinanc added 2 commits June 9, 2026 13:15

apply some suggestions

17663e9

merge next

62892af

onurinanc added 2 commits June 9, 2026 15:23

add Pausable externally

8c89940

Merge branch 'next' into fix-auth-controlled

382932b

onurinanc requested a review from bobbinth June 9, 2026 13:43

		let allowlist: BTreeSet<NoteScriptRoot> =
		[NoteScriptRoot::from_raw(Word::new([Felt::ONE; 4]))].into_iter().collect();

		/// Internal helper: adds an existing user-account fungible faucet (AuthControlled).
		fn add_existing_user_fungible_faucet(

	let allowed_script_roots: alloc::collections::BTreeSet<NoteScriptRoot> =
	let allowed_script_roots: BTreeSet<NoteScriptRoot> =

Conversation

onurinanc commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onurinanc commented Jun 3, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

onurinanc commented May 18, 2026 •

edited

Loading