chore: use Arrays#replace from OpenZeppelin v5.6.0-rc.0#31
chore: use Arrays#replace from OpenZeppelin v5.6.0-rc.0#31ernestognw wants to merge 2 commits intozkemail:mainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughUpdated OpenZeppelin contract dependencies to 5.6.0-rc.0 and refactored two verifier contracts to replace in-place memory copy operations ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/verifiers/HandleVerifier.sol (1)
148-153: Remove unused_copyTofunction.The
_copyTohelper function is no longer used after migrating tofields.replace(). This is dead code that should be removed to improve maintainability.🔎 Proposed removal
- function _copyTo(bytes32[] memory fields, uint256 offset, bytes32[] memory data) private pure { - // solhint-disable-next-line no-inline-assembly - assembly ("memory-safe") { - mcopy(add(add(0x20, fields), mul(offset, 0x20)), add(0x20, data), mul(mload(data), 0x20)) - } - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.jsonsrc/verifiers/EmailAuthVerifier.solsrc/verifiers/HandleVerifier.solsrc/verifiers/LinkEmailCommandVerifier.solsrc/verifiers/ProveAndClaimCommandVerifier.soltest/fixtures/handleCommand/HonkVerifier.soltest/src/verifiers/ClaimHandleCommandVerifier/Encode.t.soltest/src/verifiers/LinkHandleCommandVerifier/Encode.t.sol
🧰 Additional context used
🪛 GitHub Actions: CI
package.json
[error] 1-1: Prettier/formatting check failed due to code style issues detected by prettier in the repository.
src/verifiers/EmailAuthVerifier.sol
[error] 157-159: Prettier formatting changes detected in EmailAuthVerifier.sol during 'yarn fmt:check'.
src/verifiers/ProveAndClaimCommandVerifier.sol
[error] 35-40: Prettier formatting changes detected in ProveAndClaimCommandVerifier.sol. Constructor formatting differs from expected style.
src/verifiers/LinkEmailCommandVerifier.sol
[error] 30-35: Prettier formatting changes detected in LinkEmailCommandVerifier.sol. Constructor formatting differs from expected style.
test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol
[error] 45-51: Prettier formatting changes detected in Encode.t.sol for ClaimHandleCommandVerifier tests (function _assertEq signature formatting).
test/fixtures/handleCommand/HonkVerifier.sol
[error] 141-141: Prettier formatting changes detected in test fixture HonkVerifier.sol (field order comment formatting).
test/src/verifiers/LinkHandleCommandVerifier/Encode.t.sol
[error] 39-45: Prettier formatting changes detected in Encode.t.sol for LinkHandleCommandVerifier tests (function _assertEq signature formatting).
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (8)
test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol (1)
45-51: Formatting-only change: multiline function signature.The
_assertEqfunction signature has been reformatted to span multiple lines. No behavioral changes.test/src/verifiers/LinkHandleCommandVerifier/Encode.t.sol (1)
39-45: Formatting-only change: multiline function signature.The
_assertEqfunction signature has been reformatted to span multiple lines. No behavioral changes.test/fixtures/handleCommand/HonkVerifier.sol (1)
141-141: Formatting-only changes: comment and whitespace alignment.All changes in this file are formatting adjustments (comment alignment, whitespace normalization). No semantic or behavioral changes.
Also applies to: 266-269, 1167-1168, 1287-1288, 1295-1296, 1341-1344, 1371-1372, 1423-1426, 1567-1567
src/verifiers/ProveAndClaimCommandVerifier.sol (1)
35-40: Formatting-only change: multiline constructor declaration.The constructor has been reformatted to a multiline parameter list. Parameters and base constructor call remain unchanged.
src/verifiers/LinkEmailCommandVerifier.sol (1)
30-35: Formatting-only change: multiline constructor declaration.The constructor has been reformatted to a multiline parameter list. Parameters and base constructor call remain unchanged.
src/verifiers/EmailAuthVerifier.sol (2)
157-158: Formatting-only change: validation check reformatted.The field element validation has been reformatted to fewer lines. Logic remains unchanged.
185-187: LGTM: Migrated to OpenZeppelin'sArrays.replaceutility.The code now uses
fields.replace()from OpenZeppelin v5.6.0-rc.0 instead of the custom_copyTohelper. The offsets and packing logic remain unchanged, preserving the same public inputs layout.Note: The PR description mentions this change sanitizes inputs but increases gas costs. The gas diff provided shows the impact is acceptable.
Also applies to: 191-193, 195-196, 197-200
src/verifiers/HandleVerifier.sol (1)
103-112: LGTM: Migrated to OpenZeppelin'sArrays.replaceutility.The code now uses
fields.replace()from OpenZeppelin v5.6.0-rc.0 for all public input packing operations. The offsets and packing logic remain unchanged.
There was a problem hiding this comment.
Pull request overview
This PR upgrades OpenZeppelin contracts from v5.5.0 to v5.6.0-rc.0 to leverage the new Arrays.replace utility function. The custom _copyTo function in EmailAuthVerifier.sol has been removed and all array splice operations now use OpenZeppelin's standardized Arrays.replace method. While this increases gas costs slightly due to input sanitization in the OpenZeppelin implementation, it improves code maintainability by using well-tested library functions.
Key Changes:
- Upgraded OpenZeppelin contracts and contracts-upgradeable to v5.6.0-rc.0
- Replaced custom
_copyTofunction withArrays.replacefrom OpenZeppelin - Applied consistent code formatting improvements across multiple files
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated OpenZeppelin dependencies to v5.6.0-rc.0 |
| yarn.lock | Updated dependency lock file to reflect new OpenZeppelin versions |
| src/verifiers/EmailAuthVerifier.sol | Removed custom _copyTo function and replaced usages with Arrays.replace; reformatted validation logic |
| src/verifiers/HandleVerifier.sol | Replaced _copyTo calls with Arrays.replace in _packPublicInputs method |
| src/verifiers/ProveAndClaimCommandVerifier.sol | Reformatted constructor for better readability |
| src/verifiers/LinkEmailCommandVerifier.sol | Reformatted constructor for better readability |
| test/src/verifiers/LinkHandleCommandVerifier/Encode.t.sol | Reformatted _assertEq function signature |
| test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol | Reformatted _assertEq function signature |
| test/fixtures/handleCommand/HonkVerifier.sol | Improved comment formatting and indentation throughout the file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/verifiers/EmailAuthVerifier.sol (1)
184-204: Review mixing of direct assignment and replace for single-field values.The function uses direct assignment for some single-field values (lines 189-191, 195) but uses
replacefor others (line 196:isCodeExist). This inconsistency suggests:
CircomUtils.packBoolreturns abytes32[]array (requiringreplace)- Raw values like
publicKeyHashcan be directly assignedWhile this is likely correct, consider documenting why some single-field values require packing/replacement while others don't. This will help future maintainers understand the distinction.
📝 Optional: Add clarifying comment
fields[ACCOUNT_SALT_OFFSET] = publicInputs.accountSalt; + // Note: packBool returns bytes32[] even for single field, hence replace is used fields.replace(IS_CODE_EXIST_OFFSET, CircomUtils.packBool(publicInputs.isCodeExist)); + // Note: packPubKey returns bytes32[17] for the decomposed public key fields.replace(MISCELLANEOUS_DATA_OFFSET, EnsUtils.packPubKey(publicInputs.miscellaneousData));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/verifiers/EmailAuthVerifier.sol
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (4)
src/verifiers/EmailAuthVerifier.sol (4)
192-194: LGTM: Masked command packing migrated correctly.The replacement logic for
maskedCommandcorrectly uses the defined offset (12) and the packed array fromCircomUtils.packFieldsArraywith the appropriate size (605 bytes). The migration maintains functional equivalence with the removed_copyTohelper.
198-201: LGTM: Email address packing migrated correctly.The replacement logic for
emailAddresscorrectly uses the defined offset (51) and packs the final 9 fields of the 60-element public inputs array. The migration maintains the documented structure and functional equivalence.
197-197: EnsUtils.packPubKey correctly packs the DKIM public key into 17 bytes32 elements.The function returns
bytes32[] memorywith exactly 17 elements (line 77 ofsrc/utils/EnsUtils.sol), matchingMISCELLANEOUS_DATA_NUM_FIELDS. The offset at line 197 is correct and the field replacement properly handles the full 17-element array.
186-188: No issues found with theArrays.replaceusage.The migration to OpenZeppelin's
Arrays.replaceis correct. This method has been available and stable since OpenZeppelin Contracts v5.1 (October 2024), and the code properly uses it to replace array segments at specified offsets. The method is fully tested and used throughout the ecosystem. While v5.6.0-rc.0 is a release candidate, theArrays.replacefunctionality itself is not new—it was stabilized in v5.1 and remains unchanged in the RC.
1 participant
Description
Follow up to #22, but now using new OpenZeppelin's
Array.replace. We observe increased gas costs given that the utility in 5.6 sanitizes the inputs. It's fine if you consider the current implementation is better.Gas diff
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.