refactor(access): harden the shieldedaccesscontrol lib by some improvements and fixing some bugs

[0xisk]

This PR suggests some improvements, the main motivation is following OZ Solidity Access Control in the functions signatures and params names.

Refactoring

• Removed type aliases — RoleIdentifier, AdminIdentifier, and AccountIdentifier have been removed and replaced uniformly with Bytes<32>. Only RoleCommitment (the Merkle tree leaf type) is retained.

• Renamed circuits to match OZ Solidity AccessControl signatures:

  Old Name	New Name
  proveCallerRole	hasRole
  assertOnlyRole	_checkRole
  _validateRole	_hasRole

• Circuit ordering follows the Stepdown Rule — Public/exported circuits appear first, followed by the internal circuits they call. Callers are always defined before callees, so the file reads top-down without forward jumps.

• Eliminated redundant init checks — Introduced _uncheckedGrantRole / _uncheckedRevokeRole private circuits. Export wrappers add the single init check; sibling callers (grantRole, revokeRole, renounceRole) call the unchecked versions directly, giving one init check per call chain.

• Extracted _getRoleAdmin / _onlyRole — Shared internal primitives without init checks.

---

New Circuits

Circuit	Visibility	Description
hasRole(role, account: ZswapCoinPublicKey)	Public	Check whether a specific account holds a role
_checkRole(role, account: ZswapCoinPublicKey)	Internal	Revert if a specific account lacks a role
computeAccountId(role, account)	Public	Public wrapper for _computeAccountId
_onlyRole(role, accountId)	Internal	Assertion primitive; reverts if the role is not held
_uncheckedGrantRole	Private	Init-check-free grant logic
_uncheckedRevokeRole	Private	Init-check-free revoke logic

---

Documentation

• Updated all @circuitInfo annotations with current compiler output.
• Added/updated docs for all new, renamed, and internal circuits.
• Fixed doc typo: commitmentDomain → nullifierDomain.
• Fixed @param mismatch in renounceRole.
• Updated module-level usage example: assertOnlyRole → _checkRole.

---

Mock Contract

MockShieldedAccessControl.compact updated to reflect all renames, new circuits, and type alias removals. Circuit order matches the main contract.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e5851288-c34b-4cff-ae3c-c08db8fce45a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/sac-improvements

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[emnul]

The test file depends the re-export of adminRoles

[emnul]

Suggested change

	* @return {Boolean} - A boolean determining if `account` holds `role`.
	* @return {Boolean} - A boolean determining if a caller successfully proved ownership of `role`

[emnul]

Suggested change

	* @description Returns `true` if `account` holds `role` and is not revoked. The account identifier
	* @description Returns `true` if a caller proves ownership of `role` and is not revoked.The account identifier

[emnul]

Any circuit with the public key as part of its circuit signature should not be part of the public API since a consumer could accidentally dox themselves by calling it.

[emnul]

Same issue here, anyone calling this circuit would dox themselves and it should not be part of the public API

[emnul]

In general, 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 more safe which is one of our top concerns. This is especially important given that Compact doesn't have an official LSP server implementation making circuit parameter hinting impossible and super easy to mess up param inputs

[emnul]

In my opinion it's ok to deviate from the Solidity naming scheme in this case since the new method names are a better semantic fit for what's actually happening. hasRole implies we're performing a simple database 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. There's a certain degree of absolutism to hasRole. The value either exists or it doesnt and there are no failure cases in between which doesnt fit in this case.

I dont think _hasRole is a good rename for _validateRole for the same reasons described above

[emnul]

assertOnlyRole is meant to be part of the public API and should not be changed to an internal method.

[emnul]

I like the idea of an _uncheckedXXX to reduce the Initialization checks in the call chain. After thinking about it some more we can get rid of the Initialization checks in the unexported internal only circuits like _computeAccountId _validateRole etc. However I have some concerns about this: on one hand, they're internal only and not meant to be used outside the module and this should be explicitly noted in the circuit docs. On the other hand, Compact does not offer the same level of robust access controls to circuits as solidity eg private, internal, etc. Circuits are either exported or not.

[emnul]

Closing since suggestions were applied piecemeal in the target branch