Add shielded access control#420
Open
andrew-fleming wants to merge 7 commits intoOpenZeppelin:mainfrom
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces ShieldedAccessControl, a privacy-preserving role-based access control system using Merkle-tree-based role commitments and revocation nullifiers. Adds comprehensive test coverage, witness infrastructure, and simulators. Updates witness typings across the codebase to use generic ledger type parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant Contract as ShieldedAccessControl
participant Tree as Merkle Tree Ledger
participant Witness as Witness Provider
participant Nullifiers as Revocation Set
User->>Contract: canProveRole(role)
Note over Contract: Derive accountId from secret key<br/>+ instanceSalt
Contract->>Contract: Compute expected<br/>roleCommitment
Contract->>Witness: wit_getRoleCommitmentPath(roleCommitment)
Witness->>Tree: Query Merkle path<br/>for commitment
Tree-->>Witness: Return path + leaf
Witness-->>Contract: Return path
Contract->>Tree: Verify path leads to<br/>valid leaf in root
Contract->>Nullifiers: Check if nullifier<br/>is revoked
alt Not revoked & path valid
Contract-->>User: Return true
else Revoked or invalid
Contract-->>User: Return false
end
sequenceDiagram
participant Admin as Admin
participant Contract as ShieldedAccessControl
participant Tree as Merkle Tree
participant Nullifiers as Nullifier Set
Admin->>Contract: grantRole(role, accountId)
Contract->>Contract: Compute roleCommitment<br/>= H(role, accountId,<br/>instanceSalt)
Contract->>Contract: Compute nullifier<br/>= H(roleCommitment)
Contract->>Nullifiers: Check nullifier<br/>not present (not revoked)
Contract->>Tree: Insert commitment<br/>into Merkle tree
Note over Tree: Root updates
Contract-->>Admin: Confirmed
Admin->>Contract: revokeRole(role, accountId)
Contract->>Contract: Compute roleCommitment
Contract->>Contract: Compute nullifier
Contract->>Nullifiers: Insert nullifier<br/>into revocation set
Note over Nullifiers: Marks commitment as revoked
Contract-->>Admin: Confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/src/access/test/simulators/ShieldedAccessControlSimulator.ts (1)
136-141: Clone and validate the injected secret key before storing it.Line 139 keeps the caller-owned buffer by reference and accepts any length. A later mutation of that buffer silently changes the simulator identity, and non-32-byte keys violate the witness contract.
🔐 Suggested change
injectSecretKey: ( newSK: Buffer<ArrayBufferLike>, ): ShieldedAccessControlPrivateState => { - const updatedState = { secretKey: newSK }; + if (newSK.length !== 32) { + throw new Error('ShieldedAccessControl secretKey must be 32 bytes'); + } + const updatedState = { secretKey: Uint8Array.from(newSK) }; this.circuitContextManager.updatePrivateState(updatedState); return updatedState; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/access/test/simulators/ShieldedAccessControlSimulator.ts` around lines 136 - 141, The injectSecretKey implementation keeps the caller-owned buffer reference and accepts any length; change injectSecretKey to first validate that newSK is exactly 32 bytes and throw an error if not, then clone the input (e.g. create a new Buffer copy via Buffer.from(newSK) or by slicing into a new Uint8Array) and store that cloned value in the ShieldedAccessControlPrivateState before calling this.circuitContextManager.updatePrivateState(updatedState) so the simulator owns an immutable 32-byte secretKey rather than a mutable external reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/access/test/ShieldedAccessControl.test.ts`:
- Around line 136-145: The test currently uses
expect(...).not.toThrow('Initializable: contract not initialized'), which only
asserts the thrown error message isn't that specific string; update the
assertion in the test iterating circuitsNotRequiringInit (the it.each block
invoking contract[circuitName as keyof ShieldedAccessControlSimulator]) to use
expect(...).not.toThrow() so it fails if any error is thrown at all, ensuring
the "%s should succeed" cases truly do not throw.
- Around line 223-238: The test creates newContract without using the same
secret key so it may pass for the wrong reason; instantiate the second
ShieldedAccessControlSimulator with the same ADMIN_SK private key (seed) used by
contract so the only variable is instanceSalt, then call
contract._computeAccountId() and newContract._computeAccountId() to assert they
differ — ensure you pass ADMIN_SK into the ShieldedAccessControlSimulator
constructor for newContract to isolate the salt change.
- Around line 35-39: The test helper buildAccountIdHash (and the similar helper
around lines 58-62) currently passes raw UTF-8 bytes for domain strings, but the
contract pads all three domains to 32 bytes; update these helpers to explicitly
pad the accountId/nullifier domain bytes to 32 bytes (e.g., left/right pad with
zero bytes to length 32) before calling persistentHash with rt_type
(CompactTypeVector(3, CompactTypeBytes(32))) so the inputs mirror the contract’s
pad(32, ...) behavior for ACCOUNT_DOMAIN and the nullifier domain.
---
Nitpick comments:
In `@contracts/src/access/test/simulators/ShieldedAccessControlSimulator.ts`:
- Around line 136-141: The injectSecretKey implementation keeps the caller-owned
buffer reference and accepts any length; change injectSecretKey to first
validate that newSK is exactly 32 bytes and throw an error if not, then clone
the input (e.g. create a new Buffer copy via Buffer.from(newSK) or by slicing
into a new Uint8Array) and store that cloned value in the
ShieldedAccessControlPrivateState before calling
this.circuitContextManager.updatePrivateState(updatedState) so the simulator
owns an immutable 32-byte secretKey rather than a mutable external reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5299f02d-1141-468c-a90e-018431d6be88
📒 Files selected for processing (24)
CHANGELOG.mdcontracts/src/access/ShieldedAccessControl.compactcontracts/src/access/test/ShieldedAccessControl.test.tscontracts/src/access/test/mocks/MockAccessControl.compactcontracts/src/access/test/mocks/MockOwnable.compactcontracts/src/access/test/mocks/MockShieldedAccessControl.compactcontracts/src/access/test/mocks/MockZOwnablePK.compactcontracts/src/access/test/simulators/ShieldedAccessControlSimulator.tscontracts/src/access/test/simulators/ZOwnablePKSimulator.tscontracts/src/access/witnesses/ShieldedAccessControlWitnesses.tscontracts/src/access/witnesses/ZOwnablePKWitnesses.tscontracts/src/archive/test/mocks/MockShieldedToken.compactcontracts/src/security/test/mocks/MockInitializable.compactcontracts/src/security/test/mocks/MockPausable.compactcontracts/src/token/test/mocks/MockFungibleToken.compactcontracts/src/token/test/mocks/MockMultiToken.compactcontracts/src/token/test/mocks/MockNonFungibleToken.compactcontracts/src/utils/test/mocks/MockUtils.compactcontracts/test-utils/address.tspackages/simulator/README.mdpackages/simulator/test/fixtures/sample-contracts/witnesses/SampleZOwnableWitnesses.tspackages/simulator/test/fixtures/sample-contracts/witnesses/WitnessWitnesses.tspackages/simulator/test/integration/SampleZOwnableSimulator.tspackages/simulator/test/integration/WitnessSimulator.ts
0xisk
approved these changes
Apr 7, 2026
Member
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Looks good! thank you @andrew-fleming! I opened this PR for proposing improvements on the re-implmented circuits for testing which I think they are redundant as they are fully covered by their public circuits.
refactor: remove reimplemented circuits on the mocks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #190. Early commits were not signed and this would require cherry-picking commits, resigning everything, and fixing a ridiculous amount of conflicts. Credit to @emnul for most of the work and @0xisk for contributions
Resolves #88
Summary by CodeRabbit
New Features
Documentation