Update ERC4337Utils with Entrypoint v09 changes#6215
Conversation
🦋 Changeset detectedLatest commit: c1bba3c 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 |
WalkthroughThis pull request introduces a ValidationRange enum to ERC4337Utils to distinguish whether validation data timestamps are compared against block timestamps or block numbers. The parseValidationData function is updated to return a fourth value (the range), while packValidationData is overloaded to accept an optional range parameter. Additionally, support for extracting paymaster signatures from paymasterAndData is added with a new paymasterSignature function and a hasSignature flag variant for paymasterData. Tests are expanded to validate range-aware validation data handling and paymaster signature extraction across various scenarios. Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.changeset/tame-monkeys-make.md (1)
1-5: Consider documentingValidationRangeandpackValidationDatachanges.The changeset documents
paymasterSignatureandpaymasterDatachanges but omits:
- The new
ValidationRangeenum- The new
packValidationDataoverloads that accept aValidationRangeparameterThese are also new public-facing additions that users may benefit from knowing about.
CHANGELOG.md (1)
8-8: Add PR reference for consistency.Other CHANGELOG entries include PR references (e.g.,
([#5906](...))). Consider adding the PR reference for this breaking change:-- `ERC4337Utils`: The `parseValidationData` now returns a `ValidationRange` as the last return tuple value indicating whether the `validationData` is compared against a timestamp or block number. Developers must update their code to handle this new return value (e.g. `(aggregator, validAfter, validUntil) -> (aggregator, validAfter, validUntil, range)`). +- `ERC4337Utils`: The `parseValidationData` now returns a `ValidationRange` as the last return tuple value indicating whether the `validationData` is compared against a timestamp or block number. Developers must update their code to handle this new return value (e.g. `(aggregator, validAfter, validUntil) -> (aggregator, validAfter, validUntil, range)`). ([#6215](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6215))test/account/utils/draft-ERC4337Utils.test.js (1)
10-11: Consider importingPAYMASTER_SIG_MAGICfrom the helper module.This constant is duplicated here and in
test/helpers/erc4337.js(line 6). Consider exporting it from the helper and importing it here to maintain a single source of truth.-const PAYMASTER_SIG_MAGIC = '0x22e325a297439656'; +const { packValidationData, UserOperation, PAYMASTER_SIG_MAGIC } = require('../../helpers/erc4337');Note: This would require adding
PAYMASTER_SIG_MAGICto the exports intest/helpers/erc4337.js.test/helpers/erc4337.js (1)
228-236: Consider exportingPAYMASTER_SIG_MAGIC.The constant is duplicated in
test/account/utils/draft-ERC4337Utils.test.js. Adding it to the exports would centralize the definition.module.exports = { SIG_VALIDATION_SUCCESS, SIG_VALIDATION_FAILURE, + PAYMASTER_SIG_MAGIC, packValidationData, packInitCode, packPaymasterAndData, UserOperation, ERC4337Helper, };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/tame-monkeys-make.md(1 hunks).changeset/whole-turkeys-swim.md(1 hunks)CHANGELOG.md(1 hunks)contracts/account/utils/draft-ERC4337Utils.sol(6 hunks)contracts/interfaces/draft-IERC4337.sol(2 hunks)test/account/utils/draft-ERC4337Utils.test.js(12 hunks)test/helpers/enums.js(1 hunks)test/helpers/erc4337.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/crosschain/README.adoc:1-1
Timestamp: 2025-08-28T15:48:30.716Z
Learning: ernestognw prefers "Cross chain" without hyphenation rather than "Cross-chain" in documentation titles.
📚 Learning: 2025-10-15T02:52:05.027Z
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5891
File: test/account/modules/ERC7579Module.behavior.js:56-61
Timestamp: 2025-10-15T02:52:05.027Z
Learning: In ERC7579 validator tests for `isValidSignatureWithSender`, using `this.mock` (not bound to a specific account) is valid when testing signature validation with any arbitrary sender, while `this.mockFromAccount` is used when testing account-specific validation scenarios.
Applied to files:
test/account/utils/draft-ERC4337Utils.test.js
🧬 Code graph analysis (2)
test/account/utils/draft-ERC4337Utils.test.js (2)
test/helpers/erc4337.js (3)
require(1-1)require(2-2)PAYMASTER_SIG_MAGIC(6-6)test/utils/types/Time.test.js (1)
MAX_UINT48(10-10)
test/helpers/erc4337.js (1)
test/account/utils/draft-ERC4337Utils.test.js (7)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(7-7)PAYMASTER_SIG_MAGIC(10-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). (9)
- GitHub Check: halmos
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: tests-foundry
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (18)
contracts/interfaces/draft-IERC4337.sol (1)
33-35: LGTM! Documentation accurately reflects EntryPoint v0.9+ changes.The documentation clearly describes the optional
paymasterSignaturestructure and the format with sizes (2 bytes for size, 8 bytes for magic) matches the implementation indraft-ERC4337Utils.sol.Also applies to: 45-45
contracts/account/utils/draft-ERC4337Utils.sol (7)
41-48: LGTM! New constants and enum for EntryPoint v0.9+ support.The
PAYMASTER_SIG_MAGICconstant (8 bytes) andValidationRangeenum provide the foundation for the new validation range and paymaster signature features.
50-66: LGTM! Range detection logic is correctly implemented.The high-bit detection scheme (checking
>= 0x800000000000for both values) and subsequent masking with0x7FFFFFFFFFFFis a clean approach to encode the validation range within the existing 48-bit fields.
77-89: LGTM! Range-aware packing correctly mirrors the parsing logic.Setting the high bit via
| 0x800000000000forBLOCKrange aligns with the detection inparseValidationData.
120-134: LGTM! Range mismatch handling is properly implemented.The choice to use
range1when ranges differ and return a validation failure is reasonable and well-documented in the NatSpec at line 118.
137-144: LGTM! Time/block comparison correctly uses the parsed range.The ternary selection between
block.timestampandblock.numberbased onValidationRangeis implemented correctly.
222-232: Verify caller responsibility forhasSignatureflag.When
hasSignature=true, the function reads_paymasterSignatureSizeand computes the slice without verifying the presence ofPAYMASTER_SIG_MAGIC. If the caller incorrectly setshasSignature=trueon data without a signature, this could produce incorrect results.Consider adding a check for
PAYMASTER_SIG_MAGICor documenting that callers must ensure the flag matches the actual data format.
247-254: Acknowledged: function intentionally omits length validation.The NatSpec correctly documents that this private helper doesn't check minimum length. This is acceptable since it's a private function, but callers must ensure adequate length. The
paymasterSignaturefunction should add appropriate bounds checking as noted in the previous comment.test/helpers/enums.js (1)
14-14: LGTM! Test helper enum correctly mirrors Solidity definition.The
ValidationRangeenum values ('Timestamp', 'Block') correctly map to the Solidity enum ordinals (0, 1)..changeset/whole-turkeys-swim.md (1)
1-5: LGTM!The changeset correctly documents the API additions with an appropriate minor version bump. The description clearly explains the new
ValidationRangeparameter forpackValidationDatavariants and the updated return value forparseValidationData.test/account/utils/draft-ERC4337Utils.test.js (5)
40-109: LGTM!The
parseValidationDatatests comprehensively cover bothValidationRange.TimestampandValidationRange.Blockscenarios, including the edge case wherevalidUntil = 0returnsMAX_UINT48. The canonical values test also correctly expectsValidationRange.Timestamp.
112-213: LGTM!The
packValidationDatatests thoroughly cover all overload variants (address and bool) with both implicit and explicitValidationRangeparameters. The use ofethers.Typedfor overload disambiguation is correct.
261-277: LGTM!Excellent test coverage for the case where validation ranges differ. The test correctly verifies that combining data with mismatched ranges returns
SIG_VALIDATION_FAILUREwhile preserving the first validation range, and the symmetry check confirms this behavior is consistent.
343-352: LGTM!Good use of
mineUpToto test block-number-based expiration. This test correctly advances the blockchain pastvalidUntilto verify that the validation data is marked as expired.
502-534: LGTM!The paymaster signature tests comprehensively cover the
hasSignatureflag behavior and signature extraction. The test correctly verifies thatpaymasterDatawithhasSignature=falsereturns the full data including the signature blob, whilehasSignature=trueexcludes it.test/helpers/erc4337.js (3)
35-55: LGTM!The
packPaymasterAndDatafunction correctly handles the optional signature parameter. The signature blob format (signature + length + magic) appended at the end allows for easy detection and extraction of the paymaster signature.
74-74: LGTM!The
paymasterSignatureproperty is correctly added toUserOperationwith a default ofundefined, which integrates seamlessly with the updatedpackPaymasterAndDatafunction.
16-29: ConfirmValidationRangeenum values support the comparison logic.The
ValidationRangeenum intest/helpers/enums.jsis defined asEnum('Timestamp', 'Block'), which assignsTimestamp = 0andBlock = 1. This ordering correctly supports the conditionrange > ValidationRange.Timestampin thepackValidationDatafunction (line 20-21): when the range isBlock, the condition evaluates to true and applies the bit manipulation (| 0x800000000000n) to set the high bit as required by ERC-4337.
| function paymasterSignature(PackedUserOperation calldata self) internal pure returns (bytes calldata) { | ||
| if (bytes8(self.paymasterAndData[self.paymasterAndData.length - 8:]) != PAYMASTER_SIG_MAGIC) | ||
| return Calldata.emptyBytes(); | ||
|
|
||
| uint256 sigSize = _paymasterSignatureSize(self); | ||
| uint256 sigStart = self.paymasterAndData.length - sigSize - 2 - 8; | ||
| return self.paymasterAndData[sigStart:sigStart + sigSize]; | ||
| } |
There was a problem hiding this comment.
Add bounds check to prevent out-of-bounds access.
If paymasterAndData.length < 8, line 239 will revert due to out-of-bounds array access when attempting to read the last 8 bytes.
Consider adding a length check:
function paymasterSignature(PackedUserOperation calldata self) internal pure returns (bytes calldata) {
+ if (self.paymasterAndData.length < 62) return Calldata.emptyBytes(); // Minimum: 52 base + 2 size + 8 magic
if (bytes8(self.paymasterAndData[self.paymasterAndData.length - 8:]) != PAYMASTER_SIG_MAGIC)
return Calldata.emptyBytes();The minimum length of 62 ensures space for the base paymaster fields (52 bytes) plus the signature size field (2 bytes) and magic (8 bytes), even with a zero-length signature.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function paymasterSignature(PackedUserOperation calldata self) internal pure returns (bytes calldata) { | |
| if (bytes8(self.paymasterAndData[self.paymasterAndData.length - 8:]) != PAYMASTER_SIG_MAGIC) | |
| return Calldata.emptyBytes(); | |
| uint256 sigSize = _paymasterSignatureSize(self); | |
| uint256 sigStart = self.paymasterAndData.length - sigSize - 2 - 8; | |
| return self.paymasterAndData[sigStart:sigStart + sigSize]; | |
| } | |
| function paymasterSignature(PackedUserOperation calldata self) internal pure returns (bytes calldata) { | |
| if (self.paymasterAndData.length < 62) return Calldata.emptyBytes(); // Minimum: 52 base + 2 size + 8 magic | |
| if (bytes8(self.paymasterAndData[self.paymasterAndData.length - 8:]) != PAYMASTER_SIG_MAGIC) | |
| return Calldata.emptyBytes(); | |
| uint256 sigSize = _paymasterSignatureSize(self); | |
| uint256 sigStart = self.paymasterAndData.length - sigSize - 2 - 8; | |
| return self.paymasterAndData[sigStart:sigStart + sigSize]; | |
| } |
🤖 Prompt for AI Agents
In contracts/account/utils/draft-ERC4337Utils.sol around lines 238 to 245, the
code reads the last 8 bytes of paymasterAndData without length checks which will
OOB if paymasterAndData.length < 8; add a guard that first verifies
paymasterAndData.length is at least the minimum expected (use 62 as the minimum:
52 bytes base paymaster fields + 2 bytes sig size + 8 bytes magic), and if
shorter return Calldata.emptyBytes(); then compute sigSize and sigStart and
additionally validate that sigStart + sigSize <= paymasterAndData.length before
returning the slice, returning Calldata.emptyBytes() on any invalid sizes to
avoid out-of-bounds access.
| it('returns data with hasSignature = true (no signature present, too short)', async function () { | ||
| const packed = this.userOp.packed; | ||
| packed.paymasterAndData = ethers.zeroPadBytes('0x', 61); | ||
| await expect(this.utils.$paymasterData(this.userOp.packed, ethers.Typed.bool(true))).to.eventually.equal('0x'); | ||
| }); |
There was a problem hiding this comment.
Potential test isolation issue: modifying packed object.
Line 498 modifies this.userOp.packed.paymasterAndData directly. Since packed is a getter that returns a new object each time, this should be safe. However, the test then uses this.userOp.packed again on line 499, which would generate a fresh packed object (not the modified one).
Consider storing the modified packed object for the assertion:
it('returns data with hasSignature = true (no signature present, too short)', async function () {
- const packed = this.userOp.packed;
- packed.paymasterAndData = ethers.zeroPadBytes('0x', 61);
- await expect(this.utils.$paymasterData(this.userOp.packed, ethers.Typed.bool(true))).to.eventually.equal('0x');
+ const packed = this.userOp.packed;
+ packed.paymasterAndData = ethers.zeroPadBytes('0x', 61);
+ await expect(this.utils.$paymasterData(packed, ethers.Typed.bool(true))).to.eventually.equal('0x');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('returns data with hasSignature = true (no signature present, too short)', async function () { | |
| const packed = this.userOp.packed; | |
| packed.paymasterAndData = ethers.zeroPadBytes('0x', 61); | |
| await expect(this.utils.$paymasterData(this.userOp.packed, ethers.Typed.bool(true))).to.eventually.equal('0x'); | |
| }); | |
| it('returns data with hasSignature = true (no signature present, too short)', async function () { | |
| const packed = this.userOp.packed; | |
| packed.paymasterAndData = ethers.zeroPadBytes('0x', 61); | |
| await expect(this.utils.$paymasterData(packed, ethers.Typed.bool(true))).to.eventually.equal('0x'); | |
| }); |
🤖 Prompt for AI Agents
In test/account/utils/draft-ERC4337Utils.test.js around lines 496–500, the test
mutates this.userOp.packed.paymasterAndData but then reads this.userOp.packed
again (the getter returns a fresh object), so the modification is lost; instead,
create and use a local modifiedPacked object (copy packed, set paymasterAndData
to ethers.zeroPadBytes('0x', 61)) and pass that modifiedPacked into
this.utils.$paymasterData for the assertion to ensure the test uses the intended
mutated data.
…MASK if validUntil is 0
Follow up to #6135
PR Checklist
npx changeset add)