Add dynamic domain separator hash generation based on ERC-5267 #5908
Conversation
🦋 Changeset detectedLatest commit: 129a8b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Reviewer's GuideIntroduces a dynamic EIP-712 domain separator generator that reads an Class diagram for updated MessageHashUtils libraryclassDiagram
class MessageHashUtils {
+toDomainSeparator(eip712Domain: function) bytes32
-_buildDynamicTypeHash(ptr: bytes32, fields: bytes1) (typePtr: bytes32, hash: bytes32)
-_buildDynamicDomainSeparatorHash(ptr: bytes32, typePtr: bytes32, typeHash: bytes32, fields: bytes1, name: string, version: string, chainId: uint256, verifyingContract: address, salt: bytes32, extensions: uint256[]) bytes32
}
MessageHashUtils ..> Memory : uses
MessageHashUtils ..> Strings : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe update adds a changeset declaring a new minor version and introduces new MessageHashUtils helpers for constructing EIP-712 domain typehashes and separators with selectively enabled fields, aligning with ERC-5267. It adds an error ERC5267ExtensionsNotSupported and three internal pure functions: toDomainSeparator (two overloads) and toDomainTypeHash. Tests are expanded to validate all field subsets (name, version, chainId, verifyingContract, salt) against reference encoders, covering both input styles (string and pre-hashed name/version). Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/utils/cryptography/MessageHashUtils.sol (1)
181-227: Verify trailing comma removal logic and consider adding a clarifying comment.The implementation correctly constructs the EIP-712 domain type string dynamically. The trailing comma removal at line 220 uses clever arithmetic but may benefit from a comment for maintainability.
The logic works as follows:
and(fields, 0x1f)returns non-zero if any field is set- Double
iszeroconverts to boolean (0 or 1)sub(ptr, 1)when fields are present removes the trailing comma- When no fields are set,
ptrremains unchanged and ')' is added directly after '('Consider adding an inline comment at line 220 to explain the trailing comma removal:
+ // Remove trailing comma if any field is set: sub(ptr, fields != 0 ? 1 : 0) ptr := sub(ptr, iszero(iszero(and(fields, 0x1f))))test/utils/cryptography/MessageHashUtils.test.js (1)
99-148: Comprehensive test coverage, but consider adding extension bit revert test.The test suite excellently covers all 32 possible field combinations (2^5) by iterating over the bitmask. Each combination tests:
- String-based
toDomainSeparatoragainst ethers.js reference- Bytes32-based
toDomainSeparatoragainst ethers.js referencetoDomainTypeHashagainst computed type string hashThis validates that:
- Unused parameters are correctly ignored
- Both input variants produce identical results
- Empty domain (fields=0x00) works correctly
Consider adding an explicit test for the extension bit (0x20) revert case:
it('reverts on unsupported extensions', async function () { await expect( this.mock.$toDomainTypeHash(ethers.toBeHex(0x20)) ).to.be.revertedWithCustomError(this.mock, 'ERC5267ExtensionsNotSupported'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/blue-mirrors-agree.md(1 hunks)contracts/utils/cryptography/MessageHashUtils.sol(2 hunks)test/utils/cryptography/MessageHashUtils.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/utils/cryptography/MessageHashUtils.test.js (2)
test/helpers/eip712.js (1)
domain(11-17)test/helpers/random.js (1)
generators(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests-upgradeable
- GitHub Check: tests-foundry
🔇 Additional comments (5)
contracts/utils/cryptography/MessageHashUtils.sol (4)
16-16: LGTM!The error declaration follows Solidity conventions and will be used to signal unsupported ERC-5267 extensions.
102-134: LGTM!The function correctly hashes string inputs and delegates to the core implementation. This provides a convenient interface for callers who haven't pre-hashed the name and version.
136-179: LGTM!The core domain separator implementation correctly:
- Retrieves the domain type hash based on field selection
- Uses assembly for gas-efficient conditional field inclusion
- Properly aligns the fields bitmask (line 149)
- Sequentially stores enabled fields (lines 156-175)
- Computes the final hash over the exact memory range
The bit-flag logic aligns with ERC-5267 specification.
183-183: LGTM!The extension bit check (bit 5, value 0x20) correctly reverts when unsupported extensions are indicated, aligning with ERC-5267's extension mechanism.
test/utils/cryptography/MessageHashUtils.test.js (1)
5-6: LGTM!The import additions correctly bring in
domainTypeandgeneratorsneeded for the new ERC-5267 tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contracts/utils/cryptography/MessageHashUtils.sol (3)
16-16: — Consider enriching the revert for diagnosabilityInclude the offending fields in the error to simplify debugging and test assertions.
- error ERC5267ExtensionsNotSupported(); + error ERC5267ExtensionsNotSupported(bytes1 fields);And emit it at call sites:
- if (fields & 0x20 == 0x20) revert ERC5267ExtensionsNotSupported(); + if ((fields & 0x20) != 0) revert ERC5267ExtensionsNotSupported(fields);
117-134: — Avoid hashing name/version when not usedMinor gas: only keccak256 name/version when corresponding bits are set.
- function toDomainSeparator( + function toDomainSeparator( bytes1 fields, string memory name, string memory version, uint256 chainId, address verifyingContract, bytes32 salt ) internal pure returns (bytes32 hash) { - return - toDomainSeparator( - fields, - keccak256(bytes(name)), - keccak256(bytes(version)), - chainId, - verifyingContract, - salt - ); + bytes32 nameHash; + bytes32 versionHash; + if ((fields & 0x01) != 0) nameHash = keccak256(bytes(name)); + if ((fields & 0x02) != 0) versionHash = keccak256(bytes(version)); + return toDomainSeparator(fields, nameHash, versionHash, chainId, verifyingContract, salt); }
181-187: — Clarify bitwise check precedenceUse explicit parentheses or compare against zero to avoid precedence ambiguities and improve readability.
- if (fields & 0x20 == 0x20) revert ERC5267ExtensionsNotSupported(); + if ((fields & 0x20) != 0) revert ERC5267ExtensionsNotSupported(fields);.changeset/blue-mirrors-agree.md (1)
5-5: — Fix typo in changeset descriptionChange “selectivelly” → “selectively”.
-`MessageHashUtils`: Add helper functions to build EIP-712 domain typehash and separator with fields selectivelly enabled/disabled. +`MessageHashUtils`: Add helper functions to build EIP-712 domain typehash and separator with fields selectively enabled/disabled.test/utils/cryptography/MessageHashUtils.test.js (1)
99-148: — Add negative test for unsupported 0x20 flagCurrent loop covers bits 0..4 only. Add a test asserting revert with ERC5267ExtensionsNotSupported when bit 0x20 is set.
describe('ERC-5267', function () { + it('reverts on unsupported extensions bit (0x20)', async function () { + const fields = 0x20; // bit 5 + await expect(this.mock.$toDomainTypeHash(ethers.toBeHex(fields))) + .to.be.revertedWithCustomError(this.mock, 'ERC5267ExtensionsNotSupported'); + await expect( + this.mock.$toDomainSeparator( + ethers.toBeHex(fields), + ethers.Typed.string('n'), + ethers.Typed.string('v'), + 1n, + ethers.Wallet.createRandom().address, + ethers.ZeroHash, + ), + ).to.be.revertedWithCustomError(this.mock, 'ERC5267ExtensionsNotSupported'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/blue-mirrors-agree.md(1 hunks)contracts/utils/cryptography/MessageHashUtils.sol(2 hunks)test/utils/cryptography/MessageHashUtils.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/utils/cryptography/MessageHashUtils.test.js (2)
test/helpers/eip712.js (1)
domain(11-17)test/helpers/random.js (1)
generators(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests-upgradeable
- GitHub Check: tests-foundry
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (3)
contracts/utils/cryptography/MessageHashUtils.sol (2)
137-179: Assembly encoding of domain separator looks correctField order, ABI word layout, and length computation match EIP‑712; ignoring non-selected fields is correct.
189-226: Type string builder is well-formedString fragments, pointer math, trailing comma removal, and length computation are accurate.
test/utils/cryptography/MessageHashUtils.test.js (1)
5-6: Imports look correct and scoped to new testsBrings in domainType and random generators needed for ERC‑5267 cases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Fixes #4318
PR Checklist
npx changeset add)Summary by Sourcery
Add support for dynamic EIP-712 domain separator generation using ERC-5267 bit flags
New Features: