chore: remove dead _cachedChainId immutable in EIP712#61
Conversation
OverviewThis PR removes the unused QA & Test CoverageNo issues found in this domain. Web SecuritySkipped: not applicable to this PR — No web-facing endpoints or HTTP-related security concerns in this contract code. Blockchain SecurityNo issues found in this domain. PerformanceNo issues found in this domain. Stats
Automated review by Eco's specialized review team. Architecture, business logic, and correctness remain with the human reviewer. |
The _cachedChainId immutable was written in the constructor but never read. This fork of OpenZeppelin's EIP712 hardcodes the domain chain id to CROSS_CHAIN_ID = 1 for cross-chain signature compatibility, and _domainSeparatorV4() only guards the cache on address(this). OZ's block.chainid == _cachedChainId half of the cache guard was dropped, orphaning the variable. Remove it and reword the stale comment. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to remove an unused _cachedChainId immutable from the forked EIP712 implementation and update the related cache comment to reflect the intended cross-chain domain design (constant chain id).
Changes:
- Remove the unused
_cachedChainIdimmutable and its constructor assignment fromsrc/lib/EIP712.sol. - Update the EIP712 domain-separator caching comment to describe the cache keying behavior and the deliberate constant chain id.
- (Unrelated) Change
NonceManager.hashNoncesToInvalidateencoding ofsalts, which alters the resulting struct hash.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/NonceManager.sol | Changes how NoncesToInvalidate is hashed (signature/proof-impacting). |
| src/lib/EIP712.sol | Removes dead _cachedChainId and updates cache documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Cache the domain separator as an immutable value, keyed only on `address(this)` so it can be invalidated if the | ||
| // contract address changes (e.g. via proxy). The chain id is intentionally NOT part of the cache key: the domain | ||
| // deliberately uses a constant chain id (CROSS_CHAIN_ID = 1) for cross-chain signature compatibility. |
re1ro
force-pushed
the
chore/remove-dead-cached-chainid
branch
from
2990a7f to
07a9260
Compare
Summary
Removes a dead immutable left over from forking OpenZeppelin's
EIP712.src/lib/EIP712.soldeclared and wrote_cachedChainIdin the constructor but never read it. In OZ's original, the cached domain separator is invalidated when eitheraddress(this)orblock.chainidchanges. This fork intentionally drops the chain-id half of that guard — the domain hardcodesCROSS_CHAIN_ID = 1so signatures are valid across all chains (cross-chain compatibility), and_domainSeparatorV4()only comparesaddress(this) == _cachedThis. That left_cachedChainIdorphaned.Changes
_cachedChainIddeclaration and its constructor assignment.address(this)and that the constant chain id is deliberate for cross-chain compatibility.CROSS_CHAIN_IDis untouched (still used to build the domain separator and ineip712Domain()).Verification
Behavior-preserving.
forge buildsucceeds andforge testpasses: 213 tests passed, 0 failed.🤖 Generated with Claude Code