feat(policy): implement full IPolicyRegistry precompile#2769
Conversation
✅ Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
bf5e640 to
4b8ce83
Compare
4b8ce83 to
17d4976
Compare
17d4976 to
66af4c7
Compare
66af4c7 to
321a6b0
Compare
6e49305 to
e32ae12
Compare
2282a18 to
6ab6304
Compare
bb12ff8 to
1c09c04
Compare
| /// Trait for interacting with the singleton policy registry. | ||
| /// | ||
| /// Covers both authorization queries (used by every B-20 transfer/mint/redeem) | ||
| /// and administrative mutations (policy creation, member management, admin transfer). | ||
| pub trait PolicyRegistry { | ||
| /// Returns `true` if `account` is authorized under the given `policy_id`. | ||
| fn is_authorized(&self, policy_id: u64, account: Address) -> Result<bool>; | ||
|
|
||
| /// Creates a new ALLOWLIST or BLOCKLIST policy, returning its encoded ID. | ||
| fn create_policy(&mut self, admin: Address, policy_type: PolicyType) -> Result<u64>; | ||
| /// Creates a new policy and seeds it with an initial member list. | ||
| fn create_policy_with_accounts( | ||
| &mut self, | ||
| admin: Address, | ||
| policy_type: PolicyType, | ||
| accounts: alloc::vec::Vec<Address>, | ||
| ) -> Result<u64>; | ||
| /// Stages a pending admin transfer for `policy_id`. | ||
| fn stage_update_admin(&mut self, policy_id: u64, new_admin: Address) -> Result<()>; | ||
| /// Completes a pending admin transfer; caller must be the staged pending admin. | ||
| fn finalize_update_admin(&mut self, policy_id: u64) -> Result<()>; | ||
| /// Permanently relinquishes admin of `policy_id`. | ||
| fn renounce_admin(&mut self, policy_id: u64) -> Result<()>; | ||
| /// Adds or removes accounts from an ALLOWLIST policy's member set. | ||
| fn update_allowlist( | ||
| &mut self, | ||
| policy_id: u64, | ||
| allowed: bool, | ||
| accounts: alloc::vec::Vec<Address>, | ||
| ) -> Result<()>; | ||
| /// Adds or removes accounts from a BLOCKLIST policy's member set. | ||
| fn update_blocklist( | ||
| &mut self, | ||
| policy_id: u64, | ||
| blocked: bool, | ||
| accounts: alloc::vec::Vec<Address>, | ||
| ) -> Result<()>; | ||
| /// Returns the next policy ID that would be assigned for `policy_type`. | ||
| fn next_policy_id(&self, policy_type: PolicyType) -> Result<u64>; | ||
| /// Returns `true` if `policy_id` is a built-in or previously created policy. | ||
| fn policy_exists(&self, policy_id: u64) -> Result<bool>; | ||
| /// Returns the `PolicyType` of `policy_id`. | ||
| fn get_policy_type(&self, policy_id: u64) -> Result<PolicyType>; |
There was a problem hiding this comment.
The PolicyRegistry trait bundles 12 admin-mutation methods alongside the single is_authorized read. In practice, B20 tokens only ever need authorization checks — Token::policy() returns &Self::Policy, so mutating methods are inaccessible through that bound anyway. But any new consumer that takes impl PolicyRegistry gets the full mutation surface whether they need it or not.
Consider splitting into two traits:
pub trait PolicyAuthorizer {
fn is_authorized(&self, policy_id: u64, account: Address) -> Result<bool>;
}
pub trait PolicyAdmin: PolicyAuthorizer {
fn create_policy(&mut self, admin: Address, policy_type: PolicyType) -> Result<u64>;
// ... remaining mutation methods
}Then B20Token<S, P: PolicyAuthorizer> expresses the minimal bound, and PolicyRegistryStorage implements both. This eliminates the need for stub Fatal("not supported in test fake") implementations in InMemoryPolicy for methods that can never be called through the token path.
| let output = | ||
| client.call(PolicyRegistryStorage::ADDRESS, IPolicyRegistry::helloWorldCall {}).await?; | ||
| let result = IPolicyRegistry::helloWorldCall::abi_decode_returns(output.as_ref()) | ||
| client.call(PolicyRegistryStorage::ADDRESS, IPolicyRegistry::policyExistsCall { policyId: 0 }).await?; |
There was a problem hiding this comment.
Stale error context: still references helloWorld after the function was renamed to policyExists.
| client.call(PolicyRegistryStorage::ADDRESS, IPolicyRegistry::policyExistsCall { policyId: 0 }).await?; | |
| .wrap_err("Failed to decode policyExists")?; |
| let pending = self.pending_policy_admin(call.policyId)?; | ||
| Ok(IPolicyRegistry::pendingPolicyAdminCall::abi_encode_returns(&pending).into()) | ||
| } | ||
| Err(_) => Err(BasePrecompileError::UnknownFunctionSelector(selector)), |
There was a problem hiding this comment.
The Err(_) arm conflates two distinct failure modes: (1) truly unknown selector, and (2) known selector with malformed ABI arguments. SolInterface::abi_decode returns Err for both. Reporting UnknownFunctionSelector for case 2 is misleading — a caller sending createPolicy(...) with truncated args will get an error saying the function doesn't exist instead of indicating malformed calldata.
Consider attempting a selector-only match first, or at minimum adding a comment documenting that this arm intentionally covers both cases.
| .call(PolicyRegistryStorage::ADDRESS, IPolicyRegistry::policyExistsCall { policyId: 0 }) | ||
| .await?; | ||
| let result = IPolicyRegistry::policyExistsCall::abi_decode_returns(output.as_ref()) | ||
| .wrap_err("Failed to decode helloWorld")?; |
There was a problem hiding this comment.
Stale error message: the call now decodes policyExistsCall, not helloWorld.
| .wrap_err("Failed to decode helloWorld")?; | |
| .wrap_err("Failed to decode policyExists")?; |
11383b4 to
c12908e
Compare
| /// generic `P` provides the [`PolicyRegistry`] implementation consulted on | ||
| /// every transfer and mint. In production, | ||
| /// [`B20Token::with_storage_and_policy`] wires in [`crate::B20TokenStorage`] | ||
| /// and [`Policy`]. | ||
| /// and [`PolicyRegistry`]. |
There was a problem hiding this comment.
Doc comment references PolicyRegistry (the full admin trait), but the generic bound is P: Policy (the minimal read-only trait). This is misleading — readers will think the full PolicyRegistry is required when only Policy::is_authorized is used.
| /// generic `P` provides the [`PolicyRegistry`] implementation consulted on | |
| /// every transfer and mint. In production, | |
| /// [`B20Token::with_storage_and_policy`] wires in [`crate::B20TokenStorage`] | |
| /// and [`Policy`]. | |
| /// and [`PolicyRegistry`]. | |
| /// generic `P` provides the [`Policy`] implementation consulted on | |
| /// every transfer and mint. In production, | |
| /// [`B20Token::with_storage_and_policy`] wires in [`crate::B20TokenStorage`] | |
| /// and [`Policy`]. |
Resolves conflicts with main (decode_precompile_call macro, provider.rs rename). Fixes helloWorldCall references in macros.rs tests.
42fa613 to
ad1d707
Compare
| let policy_id = self.next_policy_id(policy_type)?; | ||
| let counter = policy_id & ((1u64 << 56) - 1); | ||
| let next = counter | ||
| .checked_add(1) | ||
| .filter(|&n| n < (1u64 << 56)) | ||
| .ok_or_else(|| BasePrecompileError::revert(IPolicyRegistry::CounterExhausted {}))?; | ||
| self.next_counter.write(next)?; |
There was a problem hiding this comment.
next_policy_id reads the counter, then create_policy re-reads and validates counter < (1 << 56). But the value fed into checked_add(1).filter(...) is extracted from policy_id (computed in next_policy_id), not re-read from storage. This works but is subtle — the counter read in next_policy_id and the increment here are not atomic. In the current single-threaded EVM execution model this is fine, but the indirection through next_policy_id (a pub method callers may also invoke) makes it easy to misread the code.
A clearer pattern would be to read the counter directly here rather than extracting it back out of the composed ID:
let counter = self.next_counter.read()?;
let next = counter
.checked_add(1)
.filter(|&n| n < (1u64 << 56))
.ok_or_else(|| BasePrecompileError::revert(IPolicyRegistry::CounterExhausted {}))?;
self.next_counter.write(next)?;
let policy_id = (policy_type_u8 as u64) << 56 | counter;This avoids the round-trip through next_policy_id and makes the read-increment-write sequence explicit in one place.
Nit-level, not blocking.
| pub fn renounce_admin(&mut self, policy_id: u64) -> Result<()> { | ||
| let (packed, caller) = self.require_admin(policy_id)?; | ||
| let policy_type = Self::decode_type(packed); | ||
| self.policies.at_mut(&policy_id).write(Self::encode_packed(Address::ZERO, policy_type))?; |
There was a problem hiding this comment.
renounce_admin sets the admin to Address::ZERO in the packed word, but after renouncing, require_custom still passes (the packed word is non-zero because the PolicyType byte is still set). However, require_admin will now check decode_admin(packed) != caller against Address::ZERO, meaning no one can pass the admin check — which is the intended behavior.
The subtlety is that encode_packed(Address::ZERO, policy_type) produces a non-zero packed word (the type byte is non-zero for ALLOWLIST=2 / BLOCKLIST=3), so require_custom correctly distinguishes "renounced admin" from "never created". This is correct but worth documenting in the encode_packed doc comment since the invariant "packed == 0 means never created" depends on the type byte always being non-zero for valid custom policies.
| let policy_type_u8 = policy_type as u8; | ||
| if !accounts.is_empty() { | ||
| let caller = self.storage.caller(); | ||
| for account in &accounts { | ||
| self.members.at_mut(&policy_id).at_mut(account).write(true)?; | ||
| } | ||
| match policy_type_u8 { |
There was a problem hiding this comment.
policy_type as u8 is used directly without going through as_discriminant(). This is safe today because create_policy (called on line 127) already validated the type via as_discriminant(), but it means correctness here depends on the caller having validated the type first. If someone later adds another code path that calls create_policy_with_accounts after a different validation, this as u8 cast could silently produce a wrong type byte.
Consider using the already-validated discriminant:
let policy_type_u8 = policy_type.as_discriminant()?;Or extract the type byte from the already-created policy:
let policy_type_u8 = (policy_id >> 56) as u8;Either makes the correctness self-contained rather than depending on create_policy's internal validation.
| /// Returns the EIP-3860-style input cost for calldata of the given length. | ||
| pub const fn input_cost(calldata_len: usize) -> u64 { |
There was a problem hiding this comment.
The doc comment was shortened from a detailed explanation of why G_sha3word is used (and what attack it prevents) to a terse one-liner. The original comment was valuable documentation for reviewers unfamiliar with precompile gas design — it explained why this specific rate was chosen and what happens without it. Consider preserving the rationale.
| /// Returns the EIP-3860-style input cost for calldata of the given length. | |
| pub const fn input_cost(calldata_len: usize) -> u64 { | |
| /// Returns the EIP-3860-style input cost for calldata of the given length. | |
| /// | |
| /// Charges `G_sha3word` (6 gas) per 32-byte word, rounded up — the same rate the EVM uses for | |
| /// data-processing operations (keccak256). This prevents large calldata from being free to | |
| /// process in precompiles that define their own input cost. | |
| pub const fn input_cost(calldata_len: usize) -> u64 { |
| /// Stages `new_admin` as the pending admin for `policy_id`. | ||
| /// | ||
| /// Passing `address(0)` clears a previously-staged transfer per the interface spec. | ||
| pub fn stage_update_admin(&mut self, policy_id: u64, new_admin: Address) -> Result<()> { |
There was a problem hiding this comment.
The doc comment says "Passing address(0) clears a previously-staged transfer per the interface spec" — but the ABI definition in abi.rs doesn't document this behavior. The Solidity interface just says function stageUpdateAdmin(uint64 policyId, address newAdmin) external; with no mention of zero-address semantics.
If this is part of the spec, it should also be documented in the sol! block (even as a comment) or in the PolicyRegistry trait definition in common/policy.rs. Currently a consumer reading either the ABI or the trait would not know that address(0) has special clear-pending semantics.
Also note: passing address(0) emits a PolicyAdminStaged event with newAdmin = address(0), which indexers and UIs may interpret as "staging a transfer to the zero address" rather than "clearing the pending transfer." Consider whether a separate clearPendingAdmin function or event would be more explicit.
- create_policy: read counter directly (clearer read-increment-write) - create_policy_with_accounts: use as_discriminant() instead of raw cast - encode_packed: document packed!=0 invariant for valid policies - stage_update_admin: document zero-address-clears behavior in ABI and trait - input_cost: restore gas design rationale in doc comment - handle.rs: add 6 unit tests covering Policy and PolicyRegistry trait impls
Review SummaryThorough review of the full IPolicyRegistry precompile implementation (1562 additions across 14 files). This replaces the helloWorld stub with all 13 ABI functions, storage layout, and 22 unit tests. Previous-round issues — all addressedThe prior review iteration raised ~15 issues. All have been resolved in this revision:
Architecture assessmentThe implementation is well-structured:
No new issues foundThe code is clean. Test coverage is comprehensive, covering built-in IDs, ALLOWLIST/BLOCKLIST membership, batch operations, admin transfers, static call guards, authorization checks, idempotent operations, and trait delegation. |
Motivation
The
IPolicyRegistryinterface (base-std) defines the singleton that B-20 tokens call for every transfer, mint, and redeem authorization check. This PR delivers the full precompile, replacing thehelloWorldstub.Storage namespacing: We explored both ERC-7201 sequential namespacing and per-field named slots (e.g.
keccak256("base.policy_registry.policies")) to allow field reordering across hardforks. Both approaches work with the#[contract]macro. Deferring to a follow-up PR for @refcell to decide the right convention — sequential slots (append-only) are safe for now since the precompile owns its address exclusively.What changed
IPolicyRegistryimplementation incrates/common/precompiles/src/policy/: all 13 ABI functions,SolInterfacedispatch, 4-field storage layout (slots 0-3, append-only), and 22 unit testsPolicyTypeenum:ALWAYS_ALLOW=0,ALWAYS_BLOCK=1,ALLOWLIST=2,BLOCKLIST=3; built-in IDs are 0 and 1uint8(PolicyType), low 56 bits = global counter (checked add, 56-bit cap)[167:8] = admin,[7:0] = PolicyType; packed == 0 reliably means never created