Skip to content

Conversation

@avorylli
Copy link

Add documentation note explaining that forwarded calls require at least 20 bytes of calldata for proper ERC-2771 meta-transaction processing.

Calls with shorter calldata will fall back to returning the forwarder address instead of extracting the original signer.

@avorylli avorylli requested a review from a team as a code owner October 31, 2025 23:02
@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2025

⚠️ No Changeset found

Latest commit: 4c5a4ca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

The pull request adds a NOTE to the docblock of the ERC2771Context contract. This NOTE documents that ERC-2771 forwarded calls require calldata of at least 20 bytes. The note explains that if calldata from a trusted forwarder is shorter than this requirement, the _msgSender() function will return the forwarder address instead of extracting the original signer. The documentation also addresses potential fallback behavior and calldata length assumptions. No code logic or public API changes are introduced.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "docs: clarify 20-byte calldata minimum requirement in ERC2771Context" directly and accurately describes the main change in the changeset. The raw summary confirms that the PR adds documentation notes to the ERC2771Context.sol contract docblock explaining the 20-byte calldata minimum requirement for ERC-2771 forwarded calls. The title is concise, specific, uses the appropriate "docs:" prefix for documentation changes, and clearly communicates the primary change without vague language. A reviewer scanning the PR history would immediately understand that this is a documentation clarification about calldata requirements in ERC2771Context.
Description Check ✅ Passed The pull request description is directly related to the changeset and provides meaningful information about the documentation being added. It explains that the PR adds a documentation note about the 20-byte calldata minimum requirement for ERC-2771 meta-transaction processing and describes the fallback behavior when calldata is shorter than the requirement. This matches the raw summary which confirms that the change adds explanatory notes to the ERC2771Context.sol contract docblock. The description is neither off-topic nor excessively vague, and it clearly conveys what documentation content is being added.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8631702 and bbef0e4.

📒 Files selected for processing (1)
  • contracts/metatx/ERC2771Context.sol (1 hunks)
⏰ 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: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: Pages changed - solidity-contracts
  • GitHub Check: tests-foundry
  • GitHub Check: tests-upgradeable
  • GitHub Check: slither
  • GitHub Check: coverage
  • GitHub Check: tests
  • GitHub Check: halmos
🔇 Additional comments (1)
contracts/metatx/ERC2771Context.sol (1)

11-15: LGTM! Excellent documentation addition.

This NOTE accurately describes the implementation behavior and provides valuable clarity for developers using ERC2771Context. The documentation correctly explains that:

  1. The 20-byte minimum requirement matches _contextSuffixLength() (line 94)
  2. The fallback behavior is correctly implemented in _msgSender() (lines 64-69) - when calldataLength < contextSuffixLength, the condition fails and super._msgSender() returns the forwarder's address
  3. The note complements (rather than duplicates) the existing WARNING about calldata length dependencies

This helps prevent confusion about edge cases where forwarded calls have minimal or empty calldata.


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.

@avorylli avorylli requested a review from ernestognw November 3, 2025 18:16
@ernestognw ernestognw changed the base branch from master to typo-fixes November 3, 2025 18:16
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@ernestognw ernestognw merged commit 520ade0 into OpenZeppelin:typo-fixes Nov 3, 2025
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants