-
Notifications
You must be signed in to change notification settings - Fork 12.2k
Add versions of ERC7913 validation in calldata #5998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add versions of ERC7913 validation in calldata #5998
Conversation
🦋 Changeset detectedLatest commit: 845c138 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 adds two new calldata-based signature verification functions to SignatureChecker: Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils/cryptography/SignatureChecker.test.js (1)
399-426
: Consider adding calldata variant test for unordered signatures.The test at lines 399-426 validates unordered ERC-7913 signatures but only for the memory-based
areValidSignaturesNow
function. While the ordering logic is identical between memory and calldata variants, adding a parallel test forareValidSignaturesNowCalldata
would ensure complete parity in test coverage.If desired, add a calldata variant test after line 426:
await expect( this.mock.$areValidSignaturesNowCalldata( TEST_MESSAGE_HASH, signers.map(({ signer }) => ethers.Typed.bytes(signer)), signers.map(({ signature }) => signature), ), ).to.eventually.be.true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/fine-teams-serve.md
(1 hunks)contracts/utils/cryptography/SignatureChecker.sol
(2 hunks)test/utils/cryptography/SignatureChecker.test.js
(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T02:52:05.027Z
Learnt from: ernestognw
PR: OpenZeppelin/openzeppelin-contracts#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/utils/cryptography/SignatureChecker.test.js
⏰ 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). (4)
- GitHub Check: tests-foundry
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests
🔇 Additional comments (5)
.changeset/fine-teams-serve.md (1)
1-5
: LGTM!The changeset format is correct, and the description accurately reflects the addition of calldata-based signature verification functions. A minor version bump is appropriate for these new features.
test/utils/cryptography/SignatureChecker.test.js (2)
39-45
: LGTM! Comprehensive calldata variant test coverage.The test additions properly validate the calldata-based signature verification functions across all scenarios (EOA, ERC1271, ERC7913). The use of
ethers.Typed.address()
andethers.Typed.bytes()
ensures correct type handling for calldata parameters.Also applies to: 52-58, 65-71, 135-137, 145-147, 155-157, 167-169, 177-179, 187-189, 204-205, 218-219, 228-229, 242-243, 251-252
266-268
: LGTM! Thorough batch signature validation testing.The calldata variant tests for
areValidSignaturesNowCalldata
cover all critical scenarios: single/multiple signatures, mixed signer types, invalid signatures, duplicates, and length mismatches.Also applies to: 298-304, 326-332, 354-360, 390-396, 447-453, 475-481, 503-509
contracts/utils/cryptography/SignatureChecker.sol (2)
124-144
: LGTM! Correct calldata variant implementation.The
isValidSignatureNowCalldata
function properly mirrors the memory-based version with appropriate calldata handling:
- Line 135: Correctly delegates to the calldata variant of the address-based check
- Line 138: Uses proper calldata slicing syntax
signer[20:]
instead of the memory-basedsigner.slice(20)
The calldata parameters provide gas savings compared to memory, making this a valuable addition.
187-219
: LGTM! Proper batch verification with calldata.The
areValidSignaturesNowCalldata
function correctly implements batch signature verification with calldata parameters:
- Line 200: Properly declares the local
signer
variable asbytes calldata
- Line 203: Delegates to the calldata-based single-signature verification function
- Lines 205-214: Deduplication logic remains identical to the memory version, correctly using
keccak256
on calldata bytesThe implementation maintains parity with the memory variant while benefiting from reduced gas costs.
Fixes #????
PR Checklist
npx changeset add
)