Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: ernestognw <ernestognw@gmail.com>
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…6247) Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: ernestognw <ernestognw@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…6303) Co-authored-by: Ernesto <ernestognw@Ernestos-MacBook-Pro.local> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
This reverts commit 4a8b122.
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…ing the FMP to a low value (#6348) Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…ift (#6302) Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…es empty (#6331) Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
|
Contributor
WalkthroughThis PR prepares OpenZeppelin Contracts for v5.6.0 release by removing 16 changeset files, updating version headers across contracts from v5.5.0 to v5.6.0, and bumping package versions. It renames BridgeERC20Core to BridgeFungible, refactors ERC7786Recipient to stateless design, updates Memory API (setFreeMemoryPointer to unsafeSetFreeMemoryPointer), adds ERC165 support to ERC6909 extensions, modifies TrieProof traversal logic, updates compiler versions to 0.8.31, and consolidates GitHub Actions setup. Test files are updated to reflect renamed events and new interface contracts. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/utils/cryptography/signers/SignerWebAuthn.sol (1)
13-16:⚠️ Potential issue | 🟡 MinorStale class-level NatSpec: "raw P256 signature validation" path is removed.
The class documentation (lines 13-16) still states the contract "allows for both WebAuthn and raw P256 signature validation, providing compatibility with both signature types," but the new implementation returns
falsefor any non-WebAuthn input — the raw P256 fallback viasuper._rawSignatureValidationhas been removed. The NatSpec should be updated to reflect the contract now only accepts WebAuthn authentication assertions.- * This contract enables signature validation using WebAuthn authentication assertions, - * leveraging the P256 public key stored in the contract. It allows for both WebAuthn - * and raw P256 signature validation, providing compatibility with both signature types. + * This contract enables signature validation using WebAuthn authentication assertions, + * leveraging the P256 public key stored in the contract. The signature must be a valid + * WebAuthn authentication assertion; raw P256 signatures are not accepted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/utils/cryptography/signers/SignerWebAuthn.sol` around lines 13 - 16, Update the class NatSpec for SignerWebAuthn.sol to remove the stale claim about raw P256 signature validation and state that this contract only accepts WebAuthn authentication assertions; specifically mention that the signature input is expected to be an abi-encoded WebAuthn-WebAuthnAuth struct and that non-WebAuthn inputs are rejected (the previous fallback to super._rawSignatureValidation has been removed).CHANGELOG.md (1)
71-73:⚠️ Potential issue | 🟡 MinorPre-existing duplicate bug-fix entry in the 5.5.0 section.
Lines 71 and 73 contain the identical
AccountERC7579isModuleInstalledfix entry (PR#5961). One should be removed. This is a pre-existing issue but is now visible in the context of this release PR.📝 Proposed fix
- `AccountERC7579`: Prevent revert in `isModuleInstalled` for fallback modules when `additionalContext` has fewer than 4 bytes. The function now returns `false` instead of reverting, ensuring ERC-7579 compliance. ([`#5961`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5961)) - `ERC165Checker`: Ensure the `supportsERC165` function returns false if the target reverts during the `supportsInterface(0xffffffff)` call. ([`#5880`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5880)) -- `AccountERC7579`: Prevent revert in `isModuleInstalled` for fallback modules when `additionalContext` has fewer than 4 bytes. The function now returns `false` instead of reverting, ensuring ERC-7579 compliance. ([`#5961`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5961))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 71 - 73, There is a duplicate changelog entry for the AccountERC7579 isModuleInstalled bugfix (PR `#5961`) in the 5.5.0 section; remove the redundant line so only one entry referencing `AccountERC7579` and `isModuleInstalled` (PR `#5961`) remains, keeping the other unrelated entry (`ERC165Checker` supportsERC165) untouched.contracts/crosschain/bridges/abstract/BridgeFungible.sol (1)
18-21:⚠️ Potential issue | 🟡 MinorMinor NatSpec grammar nit: "which interface with" → "which interfaces with".
Line 19:
{BridgeERC7802}, which interface with ERC-7802should readwhich interfaces with.📝 Proposed fix
- * which interface with ERC-7802 to provide an approve-free user experience. It is also used by the {ERC20Crosschain} + * which interfaces with ERC-7802 to provide an approve-free user experience. It is also used by the {ERC20Crosschain}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/crosschain/bridges/abstract/BridgeFungible.sol` around lines 18 - 21, Update the NatSpec comment in the BridgeFungible contract documentation: change the phrase "{BridgeERC7802}, which interface with ERC-7802" to "{BridgeERC7802}, which interfaces with ERC-7802" so the grammar matches the singular "which interfaces with" used for BridgeERC20/BridgeERC7802 and the ERC20Crosschain reference; modify the comment block that documents BridgeERC20, BridgeERC7802, and ERC20Crosschain accordingly.
🧹 Nitpick comments (2)
test/utils/cryptography/RSA.test.js (1)
34-34: Consider normalizing static test case exponents for consistency.The NIST loop normalizes the exponent via
ethers.stripZerosLeftfor gas savings, but the staticothers testscases (e.g.,exp: '0x010001'on line 53) retain leading-zero-padded exponents. For consistency,'0x010001'could be stripped to'0x10001'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/cryptography/RSA.test.js` at line 34, Normalize the static exponent literals in the RSA tests so they match the NIST loop's normalized form: update the hard-coded exponent values in the "others tests" cases (e.g., change the literal '0x010001' used as test.e to '0x10001') so they no longer contain leading-zero padding, ensuring consistency with the runtime normalization done via ethers.stripZerosLeft('0x' + test.e).test/utils/cryptography/TrieProof.test.js (1)
191-247: Consider extracting the duplicated inline-node proof assertions into a helper.Lines 196-215 and Lines 225-246 repeat the same setup/verification pattern with different slot fixtures. A local helper would reduce duplication and future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/cryptography/TrieProof.test.js` around lines 191 - 247, Extract the duplicated setup and verification into a small helper (e.g., runInlineNodeProofTest) that accepts the slots map and performs: create a MerklePatriciaTrie({ useKeyHashing: false }), put each slot/value via tree.put(ethers.getBytes(slot), ethers.getBytes(value)), compute const root = ethers.hexlify(tree.root()), loop over entries to build const proof = await createMerkleProof(tree, ethers.getBytes(slot)) and assert both this.mock.$verify(encodeStorageLeaf(value), root, slot, proof) and this.mock.$verify(encodeStorageLeaf(value), root, slot, proof.slice(0, -1)); then call that helper from both it blocks (support inlining in extension node and support inlining in branch node) passing their respective slots objects to remove the duplicated lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/setup/action.yml:
- Around line 106-108: The current steps download and install
"solc-static-linux" without verifying integrity or using the official source;
replace the simple wget/chmod/mv sequence with a verification flow: fetch the
official solc-bin metadata (binaries.soliditylang.org/linux-amd64/list.json) for
the version from steps.versions.outputs.solc (or, if intentionally using
argotorg/solidity, fetch that fork's signed checksum), extract the expected
SHA256 for that binary, download the binary to a temp file (the current wget
target "solc-static-linux"), compute its SHA256 and compare to the expected
value, and abort the job with a clear error if the checksums do not match; only
after a successful match run the existing chmod +x and sudo mv commands to
install /usr/local/bin/solc.
In `@contracts/interfaces/draft-IERC4337.sol`:
- Line 2: The file header string "OpenZeppelin Contracts (last updated v5.6.0)
(interfaces/draft-IERC4337.sol)" was bumped despite no substantive changes;
restore the accurate last-updated annotation (e.g., change "v5.6.0" back to
"v5.4.0") or add a brief commit-note indicating this is a deliberate
project-wide release-header normalization so downstream consumers know it's
intentional; update the header text in the top-of-file comment accordingly.
In `@contracts/utils/introspection/ERC165Checker.sol`:
- Around line 124-125: Update the NatSpec in ERC165Checker for the function that
returns (bool success, bool supported) to state explicitly that the `supported`
flag is only meaningful when `success` is true; mention that a failing call can
still produce a non-zero first word in a >=32-byte revert payload yielding
(false, true), so callers should interpret `supported` only when `success` is
true (i.e., use `success && supported`). Reference the ERC165Checker contract
and the function that returns (success, supported) so future callers understand
the correct gating.
In `@contracts/utils/RelayedCall.sol`:
- Around line 19-21: Fix the grammar in the header comment of the
RelayedCall.sol library: change the phrase "their target chain has supports for
EIP-3855" to "their target chain has support for EIP-3855" (update the comment
inside the RelayedCall.sol file near the NOTE about PUSH0/EIP-3855).
In `@test/helpers/trie.js`:
- Around line 18-22: Replace the use of tx.wait() when building the receipt trie
with fetching the receipt by hash via the transaction's provider so reverted
transactions don't throw; specifically, in the block where you call tx.wait()
and then this.receiptTrie.put(BlockTries.indexToKeyBytes(tx.index),
BlockTries.serializeReceipt(receipt)), change to
tx.provider.getTransactionReceipt(tx.hash) and then put the returned receipt
using BlockTries.serializeReceipt so all receipts (including status 0) are
included in receiptTrie.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 71-73: There is a duplicate changelog entry for the AccountERC7579
isModuleInstalled bugfix (PR `#5961`) in the 5.5.0 section; remove the redundant
line so only one entry referencing `AccountERC7579` and `isModuleInstalled` (PR
`#5961`) remains, keeping the other unrelated entry (`ERC165Checker`
supportsERC165) untouched.
In `@contracts/crosschain/bridges/abstract/BridgeFungible.sol`:
- Around line 18-21: Update the NatSpec comment in the BridgeFungible contract
documentation: change the phrase "{BridgeERC7802}, which interface with
ERC-7802" to "{BridgeERC7802}, which interfaces with ERC-7802" so the grammar
matches the singular "which interfaces with" used for BridgeERC20/BridgeERC7802
and the ERC20Crosschain reference; modify the comment block that documents
BridgeERC20, BridgeERC7802, and ERC20Crosschain accordingly.
In `@contracts/utils/cryptography/signers/SignerWebAuthn.sol`:
- Around line 13-16: Update the class NatSpec for SignerWebAuthn.sol to remove
the stale claim about raw P256 signature validation and state that this contract
only accepts WebAuthn authentication assertions; specifically mention that the
signature input is expected to be an abi-encoded WebAuthn-WebAuthnAuth struct
and that non-WebAuthn inputs are rejected (the previous fallback to
super._rawSignatureValidation has been removed).
---
Nitpick comments:
In `@test/utils/cryptography/RSA.test.js`:
- Line 34: Normalize the static exponent literals in the RSA tests so they match
the NIST loop's normalized form: update the hard-coded exponent values in the
"others tests" cases (e.g., change the literal '0x010001' used as test.e to
'0x10001') so they no longer contain leading-zero padding, ensuring consistency
with the runtime normalization done via ethers.stripZerosLeft('0x' + test.e).
In `@test/utils/cryptography/TrieProof.test.js`:
- Around line 191-247: Extract the duplicated setup and verification into a
small helper (e.g., runInlineNodeProofTest) that accepts the slots map and
performs: create a MerklePatriciaTrie({ useKeyHashing: false }), put each
slot/value via tree.put(ethers.getBytes(slot), ethers.getBytes(value)), compute
const root = ethers.hexlify(tree.root()), loop over entries to build const proof
= await createMerkleProof(tree, ethers.getBytes(slot)) and assert both
this.mock.$verify(encodeStorageLeaf(value), root, slot, proof) and
this.mock.$verify(encodeStorageLeaf(value), root, slot, proof.slice(0, -1));
then call that helper from both it blocks (support inlining in extension node
and support inlining in branch node) passing their respective slots objects to
remove the duplicated lines.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (123)
.changeset/blue-mirrors-agree.md.changeset/bright-webs-create.md.changeset/clean-worlds-end.md.changeset/flat-ideas-count.md.changeset/fluffy-facts-brake.md.changeset/forty-ads-design.md.changeset/full-emus-hear.md.changeset/fuzzy-lizards-do.md.changeset/grumpy-cats-brake.md.changeset/khaki-crews-join.md.changeset/new-socks-deny.md.changeset/shaky-phones-mix.md.changeset/social-tools-sniff.md.changeset/spotty-plums-brush.md.changeset/stale-lizards-cheat.md.changeset/swift-planets-juggle.md.changeset/tame-monkeys-make.md.changeset/tender-pans-yawn.md.changeset/thick-banks-relate.md.changeset/vast-worlds-pull.md.changeset/whole-turkeys-swim.md.changeset/yellow-clowns-mate.md.changeset/young-corners-help.md.github/actions/setup/action.yml.github/workflows/formal-verification.yml.github/workflows/release-upgradeable.ymlCHANGELOG.mdLICENSEcontracts/access/AccessControl.solcontracts/access/extensions/AccessControlDefaultAdminRules.solcontracts/access/extensions/IAccessControlDefaultAdminRules.solcontracts/access/manager/IAccessManager.solcontracts/account/Account.solcontracts/account/extensions/draft-AccountERC7579.solcontracts/account/extensions/draft-AccountERC7579Hooked.solcontracts/account/utils/EIP7702Utils.solcontracts/account/utils/draft-ERC4337Utils.solcontracts/account/utils/draft-ERC7579Utils.solcontracts/crosschain/CrosschainLinked.solcontracts/crosschain/ERC7786Recipient.solcontracts/crosschain/README.adoccontracts/crosschain/bridges/BridgeERC20.solcontracts/crosschain/bridges/BridgeERC7802.solcontracts/crosschain/bridges/abstract/BridgeFungible.solcontracts/finance/VestingWallet.solcontracts/governance/Governor.solcontracts/governance/TimelockController.solcontracts/governance/extensions/GovernorStorage.solcontracts/governance/utils/Votes.solcontracts/governance/utils/VotesExtended.solcontracts/interfaces/draft-IERC4337.solcontracts/interfaces/draft-IERC7579.solcontracts/metatx/ERC2771Context.solcontracts/metatx/ERC2771Forwarder.solcontracts/package.jsoncontracts/proxy/ERC1967/ERC1967Proxy.solcontracts/proxy/ERC1967/ERC1967Utils.solcontracts/token/ERC1155/ERC1155.solcontracts/token/ERC1155/extensions/ERC1155Supply.solcontracts/token/ERC1155/extensions/ERC1155URIStorage.solcontracts/token/ERC20/README.adoccontracts/token/ERC20/extensions/ERC20Crosschain.solcontracts/token/ERC20/extensions/ERC20FlashMint.solcontracts/token/ERC20/extensions/ERC20Wrapper.solcontracts/token/ERC20/extensions/ERC4626.solcontracts/token/ERC6909/extensions/ERC6909ContentURI.solcontracts/token/ERC6909/extensions/ERC6909Metadata.solcontracts/token/ERC6909/extensions/ERC6909TokenSupply.solcontracts/token/ERC721/ERC721.solcontracts/token/ERC721/extensions/ERC721Enumerable.solcontracts/token/ERC721/extensions/ERC721URIStorage.solcontracts/utils/Arrays.solcontracts/utils/Base64.solcontracts/utils/Bytes.solcontracts/utils/LowLevelCall.solcontracts/utils/Memory.solcontracts/utils/RLP.solcontracts/utils/RelayedCall.solcontracts/utils/Strings.solcontracts/utils/cryptography/ECDSA.solcontracts/utils/cryptography/MerkleProof.solcontracts/utils/cryptography/MessageHashUtils.solcontracts/utils/cryptography/SignatureChecker.solcontracts/utils/cryptography/TrieProof.solcontracts/utils/cryptography/WebAuthn.solcontracts/utils/cryptography/draft-ERC7739Utils.solcontracts/utils/cryptography/signers/MultiSignerERC7913.solcontracts/utils/cryptography/signers/SignerECDSA.solcontracts/utils/cryptography/signers/SignerEIP7702.solcontracts/utils/cryptography/signers/SignerWebAuthn.solcontracts/utils/cryptography/verifiers/ERC7913WebAuthnVerifier.solcontracts/utils/draft-InteroperableAddress.solcontracts/utils/introspection/ERC165Checker.solcontracts/utils/math/Math.solcontracts/utils/math/SafeCast.solcontracts/utils/structs/Accumulators.solcontracts/utils/structs/CircularBuffer.solcontracts/utils/structs/DoubleEndedQueue.solcontracts/utils/structs/EnumerableMap.solcontracts/utils/structs/EnumerableSet.solcontracts/utils/structs/Heap.soldocs/modules/ROOT/pages/utilities.adocfoundry.tomlhardhat.config.jspackage.jsonscripts/generate/templates/Arrays.jstest/account/AccountWebAuthn.test.jstest/crosschain/BridgeERC20.behavior.jstest/crosschain/ERC7786Recipient.test.jstest/helpers/trie.jstest/token/ERC20/extensions/ERC20Crosschain.test.jstest/token/ERC6909/extensions/ERC6909ContentURI.test.jstest/token/ERC6909/extensions/ERC6909Metadata.test.jstest/token/ERC6909/extensions/ERC6909TokenSupply.test.jstest/utils/Memory.t.soltest/utils/Memory.test.jstest/utils/Strings.test.jstest/utils/cryptography/P256.t.soltest/utils/cryptography/RSA.test.jstest/utils/cryptography/TrieProof.test.jstest/utils/cryptography/WebAuthn.t.soltest/utils/draft-InteroperableAddress.test.jstest/utils/introspection/SupportsInterface.behavior.js
💤 Files with no reviewable changes (24)
- .changeset/khaki-crews-join.md
- test/account/AccountWebAuthn.test.js
- .changeset/forty-ads-design.md
- .changeset/grumpy-cats-brake.md
- .changeset/fuzzy-lizards-do.md
- .changeset/bright-webs-create.md
- .changeset/young-corners-help.md
- .changeset/full-emus-hear.md
- .changeset/spotty-plums-brush.md
- .changeset/social-tools-sniff.md
- .changeset/vast-worlds-pull.md
- .changeset/clean-worlds-end.md
- .changeset/new-socks-deny.md
- .changeset/shaky-phones-mix.md
- .changeset/thick-banks-relate.md
- .changeset/tender-pans-yawn.md
- .changeset/yellow-clowns-mate.md
- .changeset/flat-ideas-count.md
- .changeset/blue-mirrors-agree.md
- .changeset/stale-lizards-cheat.md
- .changeset/fluffy-facts-brake.md
- .changeset/swift-planets-juggle.md
- .changeset/whole-turkeys-swim.md
- .changeset/tame-monkeys-make.md
Comment on lines
+124
to
125
| * * `supported`: true if the returned data indicating the interface is supported | ||
| */ |
Contributor
There was a problem hiding this comment.
Doc clarification: supported can be true even when success is false.
The updated comment ("true if the returned data indicating the interface is supported") removes the success coupling, which is technically accurate since the assembly derives supported independently of success. However, a revert with a ≥32-byte payload that happens to be non-zero at offset 0 would yield (false, true), which could surprise callers. All current call sites correctly gate on success && supported, but a future caller relying only on supported could be misled.
Consider clarifying that supported should only be interpreted when success is true:
- * * `supported`: true if the returned data indicating the interface is supported
+ * * `supported`: true if the returned data indicates the interface is supported (only meaningful when `success` is true)📝 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.
Suggested change
| * * `supported`: true if the returned data indicating the interface is supported | |
| */ | |
| * * `supported`: true if the returned data indicates the interface is supported (only meaningful when `success` is true) | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/utils/introspection/ERC165Checker.sol` around lines 124 - 125,
Update the NatSpec in ERC165Checker for the function that returns (bool success,
bool supported) to state explicitly that the `supported` flag is only meaningful
when `success` is true; mention that a failing call can still produce a non-zero
first word in a >=32-byte revert payload yielding (false, true), so callers
should interpret `supported` only when `success` is true (i.e., use `success &&
supported`). Reference the ERC165Checker contract and the function that returns
(success, supported) so future callers understand the correct gating.
Comment on lines
+19
to
+21
| * NOTE: This library uses the PUSH0 opcode that was introduced in the Shanghai hardfork. While this instruction is | ||
| * now widely supported, developers using the library on exotic chains should verify that their target chain has | ||
| * supports for EIP-3855. |
Contributor
There was a problem hiding this comment.
Grammar nit: "has supports for" → "has support for"
Line 21 reads "their target chain has supports for EIP-3855" — "supports" should be the noun "support".
✏️ Proposed fix
- * now widely supported, developers using the library on exotic chains should verify that their target chain has
- * supports for EIP-3855.
+ * now widely supported, developers using the library on exotic chains should verify that their target chain has
+ * support for EIP-3855.📝 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.
Suggested change
| * NOTE: This library uses the PUSH0 opcode that was introduced in the Shanghai hardfork. While this instruction is | |
| * now widely supported, developers using the library on exotic chains should verify that their target chain has | |
| * supports for EIP-3855. | |
| * NOTE: This library uses the PUSH0 opcode that was introduced in the Shanghai hardfork. While this instruction is | |
| * now widely supported, developers using the library on exotic chains should verify that their target chain has | |
| * support for EIP-3855. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/utils/RelayedCall.sol` around lines 19 - 21, Fix the grammar in the
header comment of the RelayedCall.sol library: change the phrase "their target
chain has supports for EIP-3855" to "their target chain has support for
EIP-3855" (update the comment inside the RelayedCall.sol file near the NOTE
about PUSH0/EIP-3855).
test/helpers/trie.js
Outdated
Comment on lines
+18
to
+22
| tx | ||
| .wait() | ||
| .then(receipt => | ||
| this.receiptTrie.put(BlockTries.indexToKeyBytes(tx.index), BlockTries.serializeReceipt(receipt)), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ethers.js v6 TransactionResponse.wait() status 0 reverted transaction behavior
💡 Result:
In ethers.js v6, a mined transaction that reverted will have a receipt with status === 0, and the typical behavior when you do:
await tx.wait();is that it rejects/throws a CALL_EXCEPTION (a CallExceptionError) rather than resolving normally. The thrown error includes the mined receipt (with status 0) on error.receipt. (docs.ethers.org)
So handle it like:
try {
const receipt = await tx.wait();
// success; receipt.status should be 1
} catch (e: any) {
if (e.code === "CALL_EXCEPTION" && e.receipt) {
const receipt = e.receipt; // mined receipt
// receipt.status === 0 (reverted)
}
throw e;
}Notes:
status: 0means the EVM reverted (state changes rolled back, but gas is still paid). (alchemy.com)- This “throw on
status === 0” is explicitly documented in ethers v5 forTransactionResponse.wait()and matches the v6CallExceptionError.receiptcontract (receipt present when mined + status 0). (docs.ethers.org)
Citations:
- 1: https://docs.ethers.org/v6/single-page/
- 2: https://www.alchemy.com/docs/revert-transactions/?utm_source=openai
- 3: https://docs.ethers.org/v5/single-page/?utm_source=openai
🏁 Script executed:
cat -n test/helpers/trie.js | head -40Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1769
Use tx.provider.getTransactionReceipt() instead of tx.wait() for receipt-trie construction.
On Line 19, tx.wait() rejects with CALL_EXCEPTION for reverted transactions (status 0). This breaks trie building for valid blocks containing failed transactions, even though their receipts exist and must be included in the receipt trie. Fetch receipts directly by hash to bypass this error and include all receipts regardless of execution status.
🔧 Suggested fix
- tx
- .wait()
- .then(receipt =>
- this.receiptTrie.put(BlockTries.indexToKeyBytes(tx.index), BlockTries.serializeReceipt(receipt)),
- ),
+ tx.provider.getTransactionReceipt(tx.hash).then(receipt => {
+ if (receipt == null) {
+ throw new Error(`Missing receipt for tx ${tx.hash}`);
+ }
+ return this.receiptTrie.put(
+ BlockTries.indexToKeyBytes(tx.index),
+ BlockTries.serializeReceipt(receipt),
+ );
+ }),📝 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.
Suggested change
| tx | |
| .wait() | |
| .then(receipt => | |
| this.receiptTrie.put(BlockTries.indexToKeyBytes(tx.index), BlockTries.serializeReceipt(receipt)), | |
| ), | |
| tx.provider.getTransactionReceipt(tx.hash).then(receipt => { | |
| if (receipt == null) { | |
| throw new Error(`Missing receipt for tx ${tx.hash}`); | |
| } | |
| return this.receiptTrie.put( | |
| BlockTries.indexToKeyBytes(tx.index), | |
| BlockTries.serializeReceipt(receipt), | |
| ); | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/trie.js` around lines 18 - 22, Replace the use of tx.wait() when
building the receipt trie with fetching the receipt by hash via the
transaction's provider so reverted transactions don't throw; specifically, in
the block where you call tx.wait() and then
this.receiptTrie.put(BlockTries.indexToKeyBytes(tx.index),
BlockTries.serializeReceipt(receipt)), change to
tx.provider.getTransactionReceipt(tx.hash) and then put the returned receipt
using BlockTries.serializeReceipt so all receipts (including status 0) are
included in receiptTrie.
Amxx
reviewed
Feb 25, 2026
Amxx
approved these changes
Feb 25, 2026
ernestognw
approved these changes
Feb 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.