test(roles): comprehensive storage and ops test suites for role management

[rayyan224]

Summary

• Adds 9 storage-layer tests in b20/storage.rs covering role_member_count/set_role_member_count for DEFAULT_ADMIN_ROLE (default zero, write/read-back, increment), has_role/set_role true and false paths, and role_admin/set_role_admin including the default-to-DEFAULT_ADMIN_ROLE and self-referential behaviour
• Adds 11 ops-layer tests in common/ops/roles.rs filling gaps identified in an OpenZeppelin AccessControl v5 compliance audit: revoke_role happy path + RoleRevoked event, revoke_role no-op semantics, grant_role no-op on duplicate, privileged bypass, renounce_role happy path + RoleRevoked event, AccessControlBadConfirmation, renounce_last_admin error paths (NotSoleAdmin, AccessControlUnauthorizedAccount), and the required dual-event ordering (RoleRevoked then LastAdminRenounced)

Test plan

• cargo test -p base-common-precompiles --lib b20::storage — 12 tests pass
• cargo test -p base-common-precompiles --lib common::ops::roles — 17 tests pass
• cargo test -p base-common-precompiles --lib — full 271 test suite passes

🤖 Generated with Claude Code

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/1	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 1 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 0 Global minimum 0 Max 1 1 1 if commit is unverified 0 Sum 1

[github-actions]

from_code returning Option<()> is an unusual API. A method named from_* conventionally returns Option<Self> (see FromStr, from_utf8, etc.). Since the struct is a unit struct with no data, Option<()> is functionally a bool.

Consider renaming to is_valid(code: &str) -> bool to better express intent, or have from_code return Option<Self> so the type carries meaning (e.g., could later hold the validated code or metadata).

Suggested change

	pub fn from_code(code: &str) -> Option<()> {
	pub fn from_code(code: &str) -> Option<Self> {

(with Some(Self) on the match arm instead of Some(()))

[github-actions]

Previously, the iso_currency import and the validation in initialize() were gated behind #[cfg(feature = "std")], meaning no_std builds skipped currency validation entirely. This PR removes that gate, so validation now runs unconditionally.

This is a correctness improvement (no reason to skip validation in no_std), but it's a behavioral change worth calling out: no_std consumers that previously passed invalid currency codes will now get InvalidCurrency reverts. If that's intentional (it should be), a note in the PR description would be helpful.

[github-actions]

This hardcoded list replaces the iso_currency crate's full ISO 4217 dataset. The curated approach makes sense for a stablecoin allowlist, but CLP (Chilean Peso) appears to be missing — it's an active circulating fiat currency (ISO 4217: 152). Worth double-checking if this omission is intentional.

Also, since this is a static allowlist that will need manual updates when ISO 4217 changes (new codes like ZWG Zimbabwe Gold were only added in 2024), consider adding a comment noting where the canonical source is and when it was last synced.

[rayyan224]

@ilikesymmetry Can you verify list lol

[ilikesymmetry]

https://github.com/base/base-std/blob/main/test/lib/ISO4217.sol#L28
List here, manually checking now

[ilikesymmetry]

Can confirm CLP should be there but is not in your list or mine. Probably my AI's fault, let's add!

[ilikesymmetry]

Having my agent do another pass and find out why it missed and if there are more

[rayyan224]

👍

[github-actions]

Review Summary

This PR adds comprehensive test coverage for the role management storage and ops layers, replaces the iso_currency crate with a hand-maintained allowlist, and makes the currency validation unconditional (no longer gated behind #[cfg(feature = "std")]).

What looks good

• role_id_constants_match_canonical_names test — Excellent addition. Verifying the hardcoded b256! hashes against keccak256("MINT_ROLE") etc. catches any copy-paste errors and documents the derivation of these constants.
• Inlining role constants into id() — Clean refactor. The old module-level const items were only used in one place; inlining reduces indirection with no behavioral change.
• Test coverage gaps filled — The new revoke_role, renounce_role, renounce_last_admin, and grant_role no-op/privileged tests cover important OpenZeppelin AccessControl compliance paths that were previously untested.
• no_std currency validation — Removing the #[cfg(feature = "std")] gate so initialize() always validates the currency code is a correctness improvement.

Findings from inline comments

Three inline comments were posted in a prior run and remain relevant:

1. currency.rs — from_code API shape: The from_code method on a unit struct returns Option<Self> where Self carries no data. An is_valid(code: &str) -> bool signature would more directly express intent.
2. storage.rs — Behavioral change for no_std: Currency validation was previously skipped in no_std builds. This PR makes it unconditional. Worth a note in the PR description since no_std consumers passing invalid codes will now get InvalidCurrency reverts.
3. currency.rs — Allowlist completeness: CLP (Chilean Peso) is a notable omission from the hardcoded list. The list would also benefit from a comment noting the canonical source and last-sync date to ease future maintenance.

No additional issues found. The test logic is correct, the refactoring is safe, and the Cargo.toml cleanup (removing iso_currency from workspace and feature gates) is consistent.