From 5a400eb5230c6c507bb77a2cf4b9590d0d455a39 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 10 Jun 2025 14:15:57 +0200 Subject: [PATCH 1/8] Add Bytes.splice, an inplace variant of Buffer.slice --- .changeset/afraid-chicken-attack.md | 5 +++++ contracts/utils/Bytes.sol | 29 +++++++++++++++++++++++++++++ test/utils/Bytes.test.js | 8 +++++--- 3 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 .changeset/afraid-chicken-attack.md diff --git a/.changeset/afraid-chicken-attack.md b/.changeset/afraid-chicken-attack.md new file mode 100644 index 00000000000..9898087c0b5 --- /dev/null +++ b/.changeset/afraid-chicken-attack.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Bytes`: Add `splice(bytes,uint256)` and `splice(bytes,uint256,uint256)`, two "in place" variants of the existing slice functions diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index 1234b845513..cd8c7b41fa2 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -99,6 +99,35 @@ library Bytes { return result; } + /** + * @dev In place slice: moves the content of `buffer`, from `start` (included) to the end of `buffer` to the start of that buffer. + * + * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead + */ + function splice(bytes memory buffer, uint256 start) internal pure returns (bytes memory) { + return splice(buffer, start, buffer.length); + } + + /** + * @dev In place slice: moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. + * + * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead + */ + function splice(bytes memory buffer, uint256 start, uint256 end) internal pure returns (bytes memory) { + // sanitize + uint256 length = buffer.length; + end = Math.min(end, length); + start = Math.min(start, end); + + // allocate and copy + assembly ("memory-safe") { + mcopy(add(buffer, 0x20), add(add(buffer, 0x20), start), sub(end, start)) + mstore(buffer, sub(end, start)) + } + + return buffer; + } + /** * @dev Reads a bytes32 from a bytes array without bounds checking. * diff --git a/test/utils/Bytes.test.js b/test/utils/Bytes.test.js index 52a1ae95e77..80caa7f8faa 100644 --- a/test/utils/Bytes.test.js +++ b/test/utils/Bytes.test.js @@ -56,8 +56,8 @@ describe('Bytes', function () { }); }); - describe('slice', function () { - describe('slice(bytes, uint256)', function () { + describe('slice & splice', function () { + describe('slice(bytes, uint256) & splice(bytes, uint256)', function () { for (const [descr, start] of Object.entries({ 'start = 0': 0, 'start within bound': 10, @@ -66,11 +66,12 @@ describe('Bytes', function () { it(descr, async function () { const result = ethers.hexlify(lorem.slice(start)); expect(await this.mock.$slice(lorem, start)).to.equal(result); + expect(await this.mock.$splice(lorem, start)).to.equal(result); }); } }); - describe('slice(bytes, uint256, uint256)', function () { + describe('slice(bytes, uint256, uint256) & splice(bytes, uint256, uint256)', function () { for (const [descr, [start, end]] of Object.entries({ 'start = 0': [0, 42], 'start and end within bound': [17, 42], @@ -81,6 +82,7 @@ describe('Bytes', function () { it(descr, async function () { const result = ethers.hexlify(lorem.slice(start, end)); expect(await this.mock.$slice(lorem, start, ethers.Typed.uint256(end))).to.equal(result); + expect(await this.mock.$splice(lorem, start, ethers.Typed.uint256(end))).to.equal(result); }); } }); From da27e80a05a533907fb986b1a604cefc1fabafe8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 27 Jun 2025 13:31:06 +0200 Subject: [PATCH 2/8] Update contracts/utils/Bytes.sol Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --- contracts/utils/Bytes.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index cd8c7b41fa2..a45bf62e704 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -100,7 +100,7 @@ library Bytes { } /** - * @dev In place slice: moves the content of `buffer`, from `start` (included) to the end of `buffer` to the start of that buffer. + * @dev Moves the content of `buffer`, from `start` (included) to the end of `buffer` to the start of that buffer. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */ @@ -109,7 +109,7 @@ library Bytes { } /** - * @dev In place slice: moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. + * @dev Moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */ From d66bc4f2442a2d642be010784f735a9608c72706 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 9 Jul 2025 14:48:39 -0600 Subject: [PATCH 3/8] Add fuzz tests to slice and splice --- test/utils/Bytes.t.sol | 87 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 test/utils/Bytes.t.sol diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol new file mode 100644 index 00000000000..5dd62504e64 --- /dev/null +++ b/test/utils/Bytes.t.sol @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol"; + +contract BytesTest is Test { + using Bytes for bytes; + + function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure { + start = bound(start, 0, buffer.length); + bytes memory result = buffer.slice(start); + + // Should return bytes from start to end + assertEq(result.length, buffer.length - start); + + // Verify content matches + for (uint256 i = 0; i < result.length; i++) { + assertEq(result[i], buffer[start + i]); + } + + // Original buffer should remain unchanged + assertEq(buffer.length, buffer.length); + for (uint256 i = 0; i < buffer.length; i++) { + assertEq(buffer[i], buffer[i]); + } + } + + function testSlice(bytes memory buffer, uint256 start, uint256 end) public pure { + bytes memory result = buffer.slice(start, end); + + // Calculate expected bounds after sanitization + uint256 sanitizedEnd = end > buffer.length ? buffer.length : end; + uint256 sanitizedStart = start > sanitizedEnd ? sanitizedEnd : start; + uint256 expectedLength = sanitizedEnd - sanitizedStart; + + assertEq(result.length, expectedLength); + + // Verify content matches when there's content to verify + for (uint256 i = 0; i < result.length; i++) { + assertEq(result[i], buffer[sanitizedStart + i]); + } + } + + function testSpliceWithStartOnly(bytes memory buffer, uint256 start) public pure { + start = bound(start, 0, buffer.length); + bytes memory originalBuffer = new bytes(buffer.length); + for (uint256 i = 0; i < buffer.length; i++) { + originalBuffer[i] = buffer[i]; + } + + bytes memory result = buffer.splice(start); + + // Result should be the same object as input (modified in place) + assertEq(result.length == buffer.length, true); + + // Should contain bytes from start to end, moved to beginning + assertEq(result.length, originalBuffer.length - start); + + // Verify content matches moved content + for (uint256 i = 0; i < result.length; i++) { + assertEq(result[i], originalBuffer[start + i]); + } + } + + function testSplice(bytes memory buffer, uint256 start, uint256 end) public pure { + bytes memory originalBuffer = new bytes(buffer.length); + for (uint256 i = 0; i < buffer.length; i++) { + originalBuffer[i] = buffer[i]; + } + + bytes memory result = buffer.splice(start, end); + + // Calculate expected bounds after sanitization + uint256 sanitizedEnd = end > originalBuffer.length ? originalBuffer.length : end; + uint256 sanitizedStart = start > sanitizedEnd ? sanitizedEnd : start; + uint256 expectedLength = sanitizedEnd - sanitizedStart; + + assertEq(result.length, expectedLength); + + // Verify content matches moved content + for (uint256 i = 0; i < result.length; i++) { + assertEq(result[i], originalBuffer[sanitizedStart + i]); + } + } +} From 7b02b79e1a2f96146326d9ade218154788342335 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 9 Jul 2025 14:49:38 -0600 Subject: [PATCH 4/8] Update changeset --- .changeset/afraid-chicken-attack.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/afraid-chicken-attack.md b/.changeset/afraid-chicken-attack.md index 9898087c0b5..9baefdc2ca7 100644 --- a/.changeset/afraid-chicken-attack.md +++ b/.changeset/afraid-chicken-attack.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Bytes`: Add `splice(bytes,uint256)` and `splice(bytes,uint256,uint256)`, two "in place" variants of the existing slice functions +`Bytes`: Add `splice(bytes,uint256)` and `splice(bytes,uint256,uint256)` functions that move a specified range of bytes to the start of the buffer and truncate it in place, as an alternative to `slice`. From ca9eac8acb7288d909f7b4f1868592b9ab5e13a1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 10 Jul 2025 11:37:44 +0200 Subject: [PATCH 5/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/utils/Bytes.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index a45bf62e704..2087e34a931 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -101,6 +101,7 @@ library Bytes { /** * @dev Moves the content of `buffer`, from `start` (included) to the end of `buffer` to the start of that buffer. + * The `end` argument is truncated to the length of the `buffer`. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */ @@ -110,6 +111,7 @@ library Bytes { /** * @dev Moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. + * The `end` argument is truncated to the length of the `buffer`. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */ From 3766fb1796137d7baec31c97a2d7c2f36ae35fdd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 10 Jul 2025 11:52:15 +0200 Subject: [PATCH 6/8] update fuzz tests --- test/utils/Bytes.t.sol | 54 +++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index 5dd62504e64..0d04d93dae4 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -3,84 +3,84 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; +import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol"; contract BytesTest is Test { using Bytes for bytes; function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure { - start = bound(start, 0, buffer.length); + bytes32 hashBefore = keccak256(buffer); bytes memory result = buffer.slice(start); + bytes32 hashAfter = keccak256(buffer); + + // Original buffer was not modified + assertEq(hashBefore, hashAfter); // Should return bytes from start to end - assertEq(result.length, buffer.length - start); + assertEq(result.length, Math.saturatingSub(buffer.length, start)); // Verify content matches - for (uint256 i = 0; i < result.length; i++) { + for (uint256 i = 0; i < result.length; ++i) { assertEq(result[i], buffer[start + i]); } - - // Original buffer should remain unchanged - assertEq(buffer.length, buffer.length); - for (uint256 i = 0; i < buffer.length; i++) { - assertEq(buffer[i], buffer[i]); - } } function testSlice(bytes memory buffer, uint256 start, uint256 end) public pure { + bytes32 hashBefore = keccak256(buffer); bytes memory result = buffer.slice(start, end); + bytes32 hashAfter = keccak256(buffer); + + // Original buffer was not modified + assertEq(hashBefore, hashAfter); // Calculate expected bounds after sanitization - uint256 sanitizedEnd = end > buffer.length ? buffer.length : end; - uint256 sanitizedStart = start > sanitizedEnd ? sanitizedEnd : start; + uint256 sanitizedEnd = Math.min(end, buffer.length); + uint256 sanitizedStart = Math.min(start, sanitizedEnd); uint256 expectedLength = sanitizedEnd - sanitizedStart; assertEq(result.length, expectedLength); // Verify content matches when there's content to verify - for (uint256 i = 0; i < result.length; i++) { + for (uint256 i = 0; i < result.length; ++i) { assertEq(result[i], buffer[sanitizedStart + i]); } } function testSpliceWithStartOnly(bytes memory buffer, uint256 start) public pure { - start = bound(start, 0, buffer.length); - bytes memory originalBuffer = new bytes(buffer.length); - for (uint256 i = 0; i < buffer.length; i++) { - originalBuffer[i] = buffer[i]; - } + bytes memory originalBuffer = bytes.concat(buffer); bytes memory result = buffer.splice(start); // Result should be the same object as input (modified in place) - assertEq(result.length == buffer.length, true); + assertEq(result, buffer); // Should contain bytes from start to end, moved to beginning - assertEq(result.length, originalBuffer.length - start); + assertEq(result.length, Math.saturatingSub(originalBuffer.length, start)); // Verify content matches moved content - for (uint256 i = 0; i < result.length; i++) { + for (uint256 i = 0; i < result.length; ++i) { assertEq(result[i], originalBuffer[start + i]); } } function testSplice(bytes memory buffer, uint256 start, uint256 end) public pure { - bytes memory originalBuffer = new bytes(buffer.length); - for (uint256 i = 0; i < buffer.length; i++) { - originalBuffer[i] = buffer[i]; - } + bytes memory originalBuffer = bytes.concat(buffer); bytes memory result = buffer.splice(start, end); + // Result should be the same object as input (modified in place) + assertEq(result, buffer); + // Calculate expected bounds after sanitization - uint256 sanitizedEnd = end > originalBuffer.length ? originalBuffer.length : end; - uint256 sanitizedStart = start > sanitizedEnd ? sanitizedEnd : start; + uint256 sanitizedEnd = Math.min(end, originalBuffer.length); + uint256 sanitizedStart = Math.min(start, sanitizedEnd); uint256 expectedLength = sanitizedEnd - sanitizedStart; assertEq(result.length, expectedLength); // Verify content matches moved content - for (uint256 i = 0; i < result.length; i++) { + for (uint256 i = 0; i < result.length; ++i) { assertEq(result[i], originalBuffer[sanitizedStart + i]); } } From 6dde96829142c032c2add3dadc68de35626be233 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 10 Jul 2025 11:55:58 +0200 Subject: [PATCH 7/8] up --- test/utils/Bytes.t.sol | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index 0d04d93dae4..709c50a486a 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -10,12 +10,11 @@ contract BytesTest is Test { using Bytes for bytes; function testSliceWithStartOnly(bytes memory buffer, uint256 start) public pure { - bytes32 hashBefore = keccak256(buffer); + bytes memory originalBuffer = bytes.concat(buffer); bytes memory result = buffer.slice(start); - bytes32 hashAfter = keccak256(buffer); // Original buffer was not modified - assertEq(hashBefore, hashAfter); + assertEq(buffer, originalBuffer); // Should return bytes from start to end assertEq(result.length, Math.saturatingSub(buffer.length, start)); @@ -27,12 +26,11 @@ contract BytesTest is Test { } function testSlice(bytes memory buffer, uint256 start, uint256 end) public pure { - bytes32 hashBefore = keccak256(buffer); + bytes memory originalBuffer = bytes.concat(buffer); bytes memory result = buffer.slice(start, end); - bytes32 hashAfter = keccak256(buffer); // Original buffer was not modified - assertEq(hashBefore, hashAfter); + assertEq(buffer, originalBuffer); // Calculate expected bounds after sanitization uint256 sanitizedEnd = Math.min(end, buffer.length); @@ -49,7 +47,6 @@ contract BytesTest is Test { function testSpliceWithStartOnly(bytes memory buffer, uint256 start) public pure { bytes memory originalBuffer = bytes.concat(buffer); - bytes memory result = buffer.splice(start); // Result should be the same object as input (modified in place) @@ -66,7 +63,6 @@ contract BytesTest is Test { function testSplice(bytes memory buffer, uint256 start, uint256 end) public pure { bytes memory originalBuffer = bytes.concat(buffer); - bytes memory result = buffer.splice(start, end); // Result should be the same object as input (modified in place) From 7c4ab7b1803b8c501e1795036f918bfa83bff2cf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 10 Jul 2025 12:00:58 +0200 Subject: [PATCH 8/8] Update Bytes.sol --- contracts/utils/Bytes.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/utils/Bytes.sol b/contracts/utils/Bytes.sol index 2087e34a931..aea9b6a0c1c 100644 --- a/contracts/utils/Bytes.sol +++ b/contracts/utils/Bytes.sol @@ -80,7 +80,7 @@ library Bytes { /** * @dev Copies the content of `buffer`, from `start` (included) to `end` (excluded) into a new bytes object in - * memory. + * memory. The `end` argument is truncated to the length of the `buffer`. * * NOTE: replicates the behavior of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice[Javascript's `Array.slice`] */ @@ -101,7 +101,6 @@ library Bytes { /** * @dev Moves the content of `buffer`, from `start` (included) to the end of `buffer` to the start of that buffer. - * The `end` argument is truncated to the length of the `buffer`. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */ @@ -110,8 +109,8 @@ library Bytes { } /** - * @dev Moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. - * The `end` argument is truncated to the length of the `buffer`. + * @dev Moves the content of `buffer`, from `start` (included) to end (excluded) to the start of that buffer. The + * `end` argument is truncated to the length of the `buffer`. * * NOTE: This function modifies the provided buffer in place. If you need to preserve the original buffer, use {slice} instead */