fix: shadowed-local warnings + zero-commitment policy actor note + style + optimizations#33
Merged
Merged
Conversation
… actors - Rename local `unlocksAt`/`policyTarget` variables that shadowed named returns (getLockStatus, _authenticate) and an outer-scope local (_authenticateK1), clearing the solc "declaration shadows" warnings. Pure renames, no behavior change. - Add an inline note to PolicyManager.execute/_enforceExternal clarifying that a zero-commitment SCOPE_POLICY actor is valid per the spec (gating is the scope bit, not the commitment) but is treated as ungated by this reference manager, since a real binding commits to nonzero keccak256 params. - Sharpen the README SCOPE_POLICY example to state that combining SENDER adds no authority the protocol will honor.
2 tasks
* style: pin compiler version, dedupe policy/self helpers, positive-framing comments
- Pin deployed contracts (AccountConfiguration, accounts, canonical
authenticators) to solc 0.8.36 and bump foundry.toml to match, so the
pragma equals the build compiler and CREATE2-derived addresses stay
reproducible; interfaces/examples keep open pragmas.
- Extract _writePolicySlots, _disableInlineSelf, _resolveExplicitActor,
and _policyTargetFor to remove duplicated policy-slot writes and
actor-resolution logic across the auth/authorize/revoke paths.
- Flatten _slicePolicy with an early return; rename the "ACTOR ELEVATED
SCOPES" divider to "ACTOR SCOPE BITS" and reword the ERC-1271 note to
positive framing (no phantom SIGNER bit).
- Refresh stale NatSpec ("written iff" -> "non-zero only when",
ActorConfig fields, SCOPE_NONCE) to match the merged EIP.
* style: use @notice for error NatSpec in example policy contracts
Per Coinbase style guide 3.B.1 (errors minimally need @notice). Convert
@dev error docs to @notice in SessionPolicy and PolicyManager, and add
@notice to the four previously-undocumented PolicyManager errors.
2 tasks
chunter-cb
changed the title
2 tasks
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
1 participant
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.
unlocksAt→unlockTimeingetLockStatus, andpolicyTarget→targetin_authenticate(shadowed the named return) and_authenticateK1(shadowed the sibling function-body local). Pure renames — no behavior change.PolicyManager.execute()/_enforceExternal()explaining that aSCOPE_POLICYactor with a zero commitment is valid per the spec (gating is the scope bit, not the commitment value) but is treated as ungated by this reference manager, because a real binding commits to nonzerokeccak256params. Matches the note already in the policies README.SCOPE_POLICYexample: combiningSENDERadds no authority the protocol will honor (the README already said "SHOULD NOT", not "MUST NOT").Test plan
forge build— clean, no "declaration shadows" warnings.forge test— 355 passed, 0 failed.