-
Notifications
You must be signed in to change notification settings - Fork 12.2k
Add splice variant that replaces a portion of the input #5995
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?
Conversation
🦋 Changeset detectedLatest commit: 6a571ca 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 |
WalkthroughThe PR adds in-place splice-with-replacement functionality. In Arrays.sol, internal overloads for address[], bytes32[], and uint256[] support splice(array, replacement) and splice(array, start, replacement), performing sanitized bounds and memory copy. In Bytes.sol, new internal functions splice(bytes, bytes) and splice(bytes, uint256, bytes) replace segments within a buffer with truncation as needed. Script templates (Arrays.js) are updated to generate these overloads. Comprehensive Solidity and JavaScript tests are added for Arrays and Bytes. Changeset entries document the minor OpenZeppelin upgrade and the new splice variants. Possibly related PRs
Suggested labels
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 (2)
contracts/utils/Bytes.sol (1)
131-139
: Documentation may mislead users about behavior.The NatSpec comment states this "replicates the behavior of Javascript's
Array.splice
", but JavaScript'sArray.splice
can insert/delete elements and change array length. This implementation only replaces elements in-place without changing the buffer length, which is a more limited behavior. Consider clarifying the documentation to explicitly state that the buffer length remains unchanged and replacement is truncated to fit.Apply this diff to clarify the documentation:
/** - * @dev Replaces the content of `buffer` with the content of `replacement`. The replacement is truncated to fit within the bounds of the buffer. + * @dev Replaces the content of `buffer` with the content of `replacement` starting at position 0. + * The buffer length remains unchanged, and the replacement is truncated to fit within the buffer bounds. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead - * NOTE: replicates the behavior of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice[Javascript's `Array.splice`] + * NOTE: Similar to Javascript's `Array.splice` but with fixed-length, in-place semantics */test/utils/Bytes.test.js (1)
94-94
: Document explicit uint256 casts for overload resolutionThe use of
ethers.Typed.uint256(start/end)
is applied consistently across indexOf, lastIndexOf, slice, and splice tests to disambiguate overloads—add a note in the PR description explaining this requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/fluffy-facts-brake.md
(1 hunks).changeset/forty-ads-design.md
(1 hunks)contracts/utils/Arrays.sol
(3 hunks)contracts/utils/Bytes.sol
(1 hunks)scripts/generate/templates/Arrays.js
(1 hunks)test/utils/Arrays.t.sol
(1 hunks)test/utils/Arrays.test.js
(1 hunks)test/utils/Bytes.t.sol
(1 hunks)test/utils/Bytes.test.js
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/utils/Arrays.test.js (1)
test/helpers/random.js (1)
generators
(3-10)
test/utils/Bytes.test.js (1)
test/utils/Arrays.test.js (27)
start
(187-187)start
(195-195)start
(214-214)start
(312-312)start
(322-322)start
(332-332)start
(342-342)start
(348-348)start
(362-362)start
(383-383)replacement
(238-238)replacement
(299-299)replacement
(308-308)longReplacement
(253-253)longReplacement
(347-347)longReplacement
(370-370)shortReplacement
(265-265)emptyReplacement
(276-276)emptyReplacement
(361-361)sameSize
(283-283)copyLength
(243-243)copyLength
(255-255)copyLength
(314-314)copyLength
(324-324)copyLength
(334-334)copyLength
(350-350)copyLength
(372-372)
🪛 LanguageTool
.changeset/forty-ads-design.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... fits within the original buffer bounds.
(QB_NEW_EN)
.changeset/fluffy-facts-brake.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...ace array modification with new content.
(QB_NEW_EN)
⏰ 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). (6)
- GitHub Check: tests
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: halmos
🔇 Additional comments (4)
contracts/utils/Bytes.sol (1)
148-159
: LGTM! Boundary handling is correct.The implementation properly sanitizes
start
and computescopyLength
to prevent buffer overflow. Themcopy
operation is memory-safe and correctly handles all edge cases including empty buffers, empty replacements, and out-of-bounds start positions.test/utils/Bytes.test.js (1)
115-281
: Excellent test coverage for the new splice variants.The test suites comprehensively cover edge cases including:
- Replacements longer/shorter than buffer
- Empty replacements and empty buffers
- Boundary conditions (start at 0, middle, end, out-of-bounds)
- Single-byte operations
The expected value computation using
Uint8Array
operations correctly mirrors the Solidity implementation's truncation behavior.test/utils/Bytes.t.sol (1)
158-210
: LGTM! Comprehensive fuzz tests for splice-with-replacement.The two new test functions properly verify:
- In-place modification semantics (same object returned)
- Buffer length preservation
- Correct sanitization of start position
- Accurate copying of replacement content
- Preservation of unchanged content before and after the replacement region
The tests follow the established pattern in the file and provide strong property-based validation through fuzzing.
test/utils/Arrays.t.sol (1)
185-366
: LGTM! Comprehensive test coverage for all array types.The new test functions provide thorough validation of splice-with-replacement for
address[]
,bytes32[]
, anduint256[]
arrays. Each test suite properly verifies:
- In-place modification
- Length preservation
- Sanitized bounds
- Correct replacement content
- Unchanged content outside the replacement region
The tests follow consistent patterns and provide strong fuzz-based validation across all supported array element types.
Fixes #????
PR Checklist
npx changeset add
)