Shielded AccessControl #190

0xisk · 2026-03-12T09:12:42Z

Thank you @emnul! Looking good! I have some points here and opened this refactor PR to fix them: #382

Type aliases add unnecessary indirection — RoleIdentifier, AdminIdentifier, and AccountIdentifier are thin aliases over Bytes<32> and don't carry enough semantic weight to justify the abstraction. Replaced uniformly with Bytes<32> in the refactor; only RoleCommitment (the Merkle leaf type) is meaningful enough to retain.

Circuit names and signatures diverge from OZ Solidity AccessControl conventions

proveCallerRole, assertOnlyRole, and _validateRole don't map to anything a consumer of the Solidity interface would recognize. Beyond renaming, OZ's AccessControl also exposes two variants of each check — one for the caller and one for an arbitrary account. The current code only supported the caller variant:

before after ────────────────────────── ────────────────────────────────────── proveCallerRole(role) hasRole(role) hasRole(role, account) assertOnlyRole(role) _checkRole(role) _checkRole(role, account) _validateRole(role, accountId) _hasRole(role, accountId)

Supporting the two-argument overloads also required extracting _onlyRole(role, accountId) as a shared private primitive (which also solves point 5) — the old assertOnlyRole had identity derivation (ownPublicKey()) baked in and could not be reused for an arbitrary account. The two _checkRole overloads both delegate to _onlyRole after computing the accountId from their respective inputs:

_checkRole(role) _checkRole(role, account) │ │ _computeAccountId(role, _computeAccountId(role, ownPublicKey()) account) │ │ └───────────────┬───────────────┘ │ _onlyRole(role, accountId) │ assert(_hasRole(role, accountId), "unauthorized account")

_computeAccountId exposes the nonce as a parameter, breaking encapsulation — The original signature (zcpk, nonce) requires callers to explicitly pass the secret nonce, spreading witness handling across call sites. The refactor changes the signature to (role, account) and fetches the nonce internally via wit_secretNonce(role), making the witness boundary consistent and reducing the risk of misuse.

No public wrapper for account ID computation — _computeAccountId was purely internal with no way to call it from a top-level contract. Added computeAccountId as an exported wrapper.

Redundant init checks fired repeatedly down the call chain In the current code, nearly every circuit called Initializable_assertInitialized() independently — including internal helpers. This meant a single grantRole call triggered the check 6 times:

grantRole ├─ Initializable_assertInitialized() ← #1 ├─ getRoleAdmin(role) │ └─ Initializable_assertInitialized() ← #2 ├─ assertOnlyRole(...) │ ├─ Initializable_assertInitialized() ← #3 │ └─ proveCallerRole(role) │ ├─ Initializable_assertInitialized() ← #4 │ └─ _validateRole(...) │ └─ Initializable_assertInitialized() ← #5 └─ _grantRole(role, accountId) └─ Initializable_assertInitialized() ← #6

The refactor extracts _getRoleAdmin and _uncheckedGrantRole / _uncheckedRevokeRole as init-check-free private primitives. The single check lives only in the public export wrapper, so the same call now fires it exactly once:

grantRole ├─ Initializable_assertInitialized() ← #1 only ├─ _getRoleAdmin(role) ← no check ├─ _onlyRole(...) ← no check └─ _uncheckedGrantRole(role, accountId) ← no check

Circuit ordering requires forward-jumping to follow the logic — helpers were defined before the public circuits that call them. Reorganized to follow the Stepdown Rule: public circuits first, then the internal circuits they delegate to. This does not follow the Solidity ordering as in Solidity the order is putting all the public functions first and the internal functions at the end, IMO the Stepdown rule makes reading the code way easier and streamlined.

As mentioned above all of those points are resolved in the refactor PR.

Re-posting my thoughts from the PR introduced here:

I'm not sold on getting rid of the nominally typed aliases. I think forcing consumers to pass Bytes<32> values as RoleIdentifier, AccountId etc makes interacting with the contract's API more safe which is one of our top concerns. It adds an extra "are you sure about that?" to every call to the Shielded module. Compact also doesn't have the same tooling maturity as other languages like an official LSP implementation for parameter hinting, go to definition etc. All we really have is the formatter and the compiler so we should use those to maximum effect to make the contracts as safe as possible.

a) In my opinion it's ok to deviate from the Solidity AccessControl naming scheme in this case. The new method names are a better semantic fit for what's actually happening. hasRole implies we're performing a simple lookup but that's not what's happening under the hood. The caller is proving they have knowledge of a Merkle path to a specific commitment in the public Merkle tree. Even if the caller has the role the circuit can still fail with bad witness values. The behavior and assumptions are quite different and the API should reflect that.

b) hasRole(role, account) and _checkRole(role, account) violate the privacy guarantees of the module by exposing ZswapCoinPublicKey in the public ledger transcript and should not be part of the public API. In general, I don't think it makes sense to have circuits that prove authorization / gate roles for arbitrary accounts when account identities are tightly coupled to a user key and a secret nonce that external accounts shouldnt have knowledge of in the first place.

I updated _computeAccountId to further simplify the signature.

_computeAccountId should remain internal. Exporting the circuit increases the risk that a module consumer would build flows that leak the AccountId metadata and increase linkability

I'm mixed on the internal init check free circuits. I'm leaning towards adding them and think they're fine so long as we're really loud about keeping them internal, but would like @andrew-fleming 's opinion in case I'm missing anything. I'm not sure if I'm being overly concerned with the language limitations described in the comment below Shielded AccessControl #190 (comment)

Is this what you mean?

grantRole |_ _grantRole |__ _uncheckedGrantRole

Thank you @emnul for the good points.

I agree with you! That's a good point: refactor(access): harden the shieldedaccesscontrol lib by some improvements and fixing some bugs #382 (comment) 👍

a) Fair point on the semantic gap and I agree in principle, but I'm not fully convinced deviating is the right call, @andrew-fleming wdyt?

b) violate the privacy guarantees of the module by exposing ZswapCoinPublicKey in the public ledger transcript How is this happening? I checked the code and the only disclose was for the _computeAccountId, however, that does not revel a local param account if it does it would have got a disclose error. Correct me here if I'm wrong, that's according to my understanding to this:
Docs: all data that is not a ledger field and is not an argument or return value of a ledger operation is kept confidential

I don't think it makes sense to have circuits that prove authorization / gate roles for arbitrary accounts when account identities are tightly coupled to a user key and a secret nonce that external accounts shouldnt have knowledge of in the first place.

That's a fair point — As one motivation is to follow the Sol interface signature, another one is if there are scenarios where a batch API accepting arbitrary accounts could be useful. For example, an admin managing multiple operator keys (e.g., a DAO treasury) might want to grant/check/revoke roles for several accounts in a single circuit call rather than signing separately for each.
However, the current hasRole/_checkRole design doesn't actually enable this. The wit_secretNonce(role) witness returns a single nonce per role, so even though you can pass an arbitrary ZswapCoinPublicKey, you can't supply different nonces for different accounts in the same call. A true batch pattern would require a different circuit design that takes (account, nonce) pairs as explicit parameters.
So I agree these should be removed from the public API for now. The batch use case is worth keeping in mind as a future consideration, but it would need a purpose-built circuit design rather than the current single-account API. @andrew-fleming wdyt?

👍

The user's should have this primitive to be able to recalculate their AccountId hash locally in case if they have forgotte/lost it, I think it's important utility to be used, and for the security of using it it should be kept on the developer's responsibility.

Just let's make sure to be optimal as we can so that one init assert per exported circuit.

Yup, however, that\s more of a personal pref which not sure if it is followed in the other modules so if you guys @andrew-fleming like it we can start following it.

RE: 2b - we may have stumbled onto a disclosure bug in the Compact compiler. Check out this minimal-example I created (sorry for my spaghetti code 😅). If you scroll all the way to the bottom of test.compact you can see that the compiler definitely doesn't enforce a disclosure of account. However, if you run the test script npm run test you can see it does appear in the public transcript. If we think about it from first principles, this makes sense. A verifier cannot be convinced of the validity of a proof for a set of public inputs without having knowledge of the public inputs themselves

So I agree these should be removed from the public API for now. The batch use case is worth keeping in mind as a future consideration, but it would need a purpose-built circuit design rather than the current single-account API

Totally agree, enabling batching would be it's own can of worms esp in the ZK context where everything must be deterministic.

Circuit names and signatures diverge from OZ Solidity AccessControl conventions
proveCallerRole, assertOnlyRole, and _validateRole don't map to anything a consumer of the Solidity interface would recognize. Beyond renaming, OZ's AccessControl also exposes two variants of each check — one for the caller and one for an arbitrary account. The current code only supported the caller variant...

a) In my opinion it's ok to deviate from the Solidity AccessControl naming scheme in this case. The new method names are a better semantic fit for what's actually happening. hasRole implies we're performing a simple lookup but that's not what's happening under the hood. The caller is proving they have knowledge of a Merkle path to a specific commitment in the public Merkle tree. Even if the caller has the role the circuit can still fail with bad witness values. The behavior and assumptions are quite different and the API should reflect that.

a) Fair point on the semantic gap and I agree in principle, but I'm not fully convinced deviating is the right call, @andrew-fleming wdyt?

Sooooo I think it'd be nice to maintain the canonical API because it's familiar and we're not reinventing the wheel. That said, the changed circuit names AFAICT do better reflect the behavior and I agree that the assumptions and behaviors are different. If we stick with the og naming, it borders on being misleading for the sake of familiarity IMO. Taking a good faith perspective, the changed names sort of signal that some circuits are different from the expectation (hopefully further incentivizing users to read the docs)

Note also that the sol impl isn't a ratified standard like EIP20 so we're not breaking a standardized spec (even though following EIPs in this network is not always the best thing)

_computeAccountId should remain internal. Exporting the circuit increases the risk that a module consumer would build flows that leak the AccountId metadata and increase linkability

The user's should have this primitive to be able to recalculate their AccountId hash locally in case if they have forgotte/lost it, I think it's important utility to be used, and for the security of using it it should be kept on the developer's responsibility.

It's not possible to do this locally tho because the hashing scheme includes a reference to the instanceSalt ledger variable. Anyone calling this circuit would leak their accountId on-chain which conflicts with the goals of this module (minimizing metadata leakage). I understand the concern with providing a way to recover an AccountId in cases where it's lost or forgotten, but this should not be done at the smart contract layer imo. This should be done locally in the TypeScript / witness layer and is ultimately the Dapp developer's responsibility to provide a utility for users to recover their accountIds

Not sure if you cann't run this locally, I mean if you fetch the public state and you run the circuit locally and get the hash, you don't have to proceed with forwarding this into a tx, you can just stop here. 🤔

If I'm wrong on no.1 (which I doubt) then maybe adding salt as a param so that the user can fetch the _instanceSalt state and then run this circuit locally by passing it.

I was also on the fence about this one specifically too. An AdminIdentifier IS a RoleIdentifier but unfortunately I could not express that in the language so we have to do that clunky cast

Yeah, I'm not suggesting we create a relationship between the types. I'm just curious on the benefit of having both. I assume we think API clarity is improved with having both?

I was also on the fence about this one specifically too. An AdminIdentifier IS a RoleIdentifier but unfortunately I could not express that in the language so we have to do that clunky cast

Yeah, I'm not suggesting we create a relationship between the types. I'm just curious on the benefit of having both. I assume we think API clarity is improved with having both?

I'm not convinced there's a huge benefit to API clarity with AdminIdentifier so I removed it and replaced with another RoleIdentifier

_computeAccountId should remain internal. Exporting the circuit increases the risk that a module consumer would build flows that leak the AccountId metadata and increase linkability

The user's should have this primitive to be able to recalculate their AccountId hash locally in case if they have forgotte/lost it, I think it's important utility to be used, and for the security of using it it should be kept on the developer's responsibility.

It's not possible to do this locally tho because the hashing scheme includes a reference to the instanceSalt ledger variable. Anyone calling this circuit would leak their accountId on-chain which conflicts with the goals of this module (minimizing metadata leakage). I understand the concern with providing a way to recover an AccountId in cases where it's lost or forgotten, but this should not be done at the smart contract layer imo. This should be done locally in the TypeScript / witness layer and is ultimately the Dapp developer's responsibility to provide a utility for users to recover their accountIds

Not sure if you cann't run this locally, I mean if you fetch the public state and you run the circuit locally and get the hash, you don't have to proceed with forwarding this into a tx, you can just stop here. 🤔

If I'm wrong on no.1 (which I doubt) then maybe adding salt as a param so that the user can fetch the _instanceSalt state and then run this circuit locally by passing it.

This isn't enforceable and can be easily misused. This feels like a huge foot gun to me

~~I'm curious if https://github.com/LFDT-Minokawa/compact/blob/HEAD/CHANGELOG.md#added-2 would provide the expressiveness we're looking for~~ Nah, doesn't really do what I thought it would.

-Original file line number
+Diff line change
@@ Expand Up @@
     - Add defensive Buffer copy to ZOwnablePKWitnesses (#397)
     - Disclose commitment instead of raw owner id in `_transferOwnership` in ZOwnablePK (#397)
+    - Use generic ledger type in ZOwnablePKWitnesses (#389)
     - Bump compact compiler to v0.29.0 (#366)
     ## 0.0.1-alpha.1 (2025-12-2)
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shielded AccessControl #190

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

0xisk Mar 12, 2026

Uh oh!

emnul Mar 12, 2026 •

edited

Loading

Uh oh!

0xisk Mar 13, 2026 •

edited

Loading

Uh oh!

emnul Mar 13, 2026

Uh oh!

emnul Mar 13, 2026

Uh oh!

andrew-fleming Mar 13, 2026

Uh oh!

0xisk Mar 13, 2026

Uh oh!

andrew-fleming Mar 13, 2026

Uh oh!

emnul Mar 13, 2026

Uh oh!

emnul Mar 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shielded AccessControl #190

Are you sure you want to change the base?

Uh oh!

Shielded AccessControl #190

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

0xisk Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

emnul Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xisk Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emnul Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

emnul Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-fleming Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

0xisk Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-fleming Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

emnul Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

emnul Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emnul Mar 12, 2026 •

edited

Loading

0xisk Mar 13, 2026 •

edited

Loading

emnul Mar 13, 2026 •

edited

Loading