Skip to content

Conversation

ernestognw
Copy link
Member

Fixes #????

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@ernestognw ernestognw requested a review from a team as a code owner October 16, 2025 22:34
Copy link

changeset-bot bot commented Oct 16, 2025

🦋 Changeset detected

Latest commit: 845c138

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

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

Copy link

coderabbitai bot commented Oct 16, 2025

Walkthrough

This pull request adds two new calldata-based signature verification functions to SignatureChecker: isValidSignatureNowCalldata and areValidSignaturesNowCalldata. These functions support ERC-7913 signature verification and parallel their existing memory-based counterparts. The functions handle signer validation, delegating to address-based checks when appropriate or performing staticcalls to IERC7913SignatureVerifier. The batch variant includes deduplication checks using keccak256-hashed identifiers. Test coverage has been updated with proper type annotations using ethers.Typed. A changeset indicates a minor version update.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The PR description provided is essentially a blank template containing only placeholder text, guidance comments, a "Fixes #????" placeholder that was not filled in, and an unchecked PR checklist. It contains no substantive content describing any aspect of the changeset, including what was changed, why it was changed, or how the new calldata-based functions work. This makes the description extremely vague and generic, conveying no meaningful information about the actual changes to the SignatureChecker contract or the rationale behind the implementation. The author should provide a substantive description of the changes, such as explaining why calldata-based variants of ERC-7913 validation were added, what benefits they provide, and any relevant implementation details. At minimum, the description should acknowledge the changes shown in the changeset and fill in the issue reference if one exists. This will help reviewers and future maintainers understand the context and purpose of the PR.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Add versions of ERC7913 validation in calldata" is fully related to the main changes in the changeset. The raw_summary confirms that the PR adds two new calldata-based functions (isValidSignatureNowCalldata and areValidSignaturesNowCalldata) for ERC-7913 signature verification in SignatureChecker.sol. The title clearly summarizes this main change, allowing readers to understand that calldata variants of ERC-7913 validation are being introduced. Although there is a minor formatting issue with a double space after "Add," the core message is clear and directly maps to the changeset's primary objective.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ernestognw ernestognw mentioned this pull request Oct 16, 2025
3 tasks
Copy link

@coderabbitai coderabbitai bot left a 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 for areValidSignaturesNowCalldata 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf1377 and 845c138.

📒 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() and ethers.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-based signer.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 as bytes 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 bytes

The implementation maintains parity with the memory variant while benefiting from reduced gas costs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant