-
Notifications
You must be signed in to change notification settings - Fork 12.1k
Add Bytes.concat #5882
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
Add Bytes.concat #5882
Conversation
🦋 Changeset detectedLatest commit: bbda17f 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 |
Reviewer's GuideIntroduces a new Bytes.concat function to merge an array of byte buffers into a single bytes object, along with targeted unit tests and a changeset entry. Class diagram for the updated Bytes library with concat functionclassDiagram
class Bytes {
<<library>>
+concat(buffers: bytes[]) bytes
+equal(a: bytes, b: bytes) bool
...
}
Flow diagram for Bytes.concat function logicflowchart TD
A[Start] --> B[Initialize length = 0]
B --> C[Loop over buffers to sum lengths]
C --> D[Create result bytes of total length]
D --> E[Set offset = 0x20]
E --> F[Loop over buffers]
F --> G[Copy buffer into result at offset]
G --> H[Increment offset by buffer length]
H --> I{All buffers processed?}
I -- No --> F
I -- Yes --> J[Return result]
J --> K[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider wrapping the
offset += input.length
arithmetic inside anunchecked
block to avoid redundant overflow checks and save gas. - Add a test case where some buffers are empty and others are non‐empty (e.g. interleaved zero‐length elements) to ensure correct concatenation behavior in edge cases.
- Double‐check that the
mcopy
memory‐safe assembly macro is supported on all targeted Solidity compiler versions and consider providing a fallback memcpy implementation if not.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the `offset += input.length` arithmetic inside an `unchecked` block to avoid redundant overflow checks and save gas.
- Add a test case where some buffers are empty and others are non‐empty (e.g. interleaved zero‐length elements) to ensure correct concatenation behavior in edge cases.
- Double‐check that the `mcopy` memory‐safe assembly macro is supported on all targeted Solidity compiler versions and consider providing a fallback memcpy implementation if not.
## Individual Comments
### Comment 1
<location> `contracts/utils/Bytes.sol:134` </location>
<code_context>
+ /**
+ * @dev Concatenate an array of bytes into a single bytes object.
+ *
+ * For fixed bytes types, we recommand using the solidity built-in `bytes.concat` or (equivalent)
+ *`abi.encodePacked`.
+ *
</code_context>
<issue_to_address>
Typo in documentation: 'recommand' should be 'recommend'.
Fixing the typo enhances documentation quality.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
* For fixed bytes types, we recommand using the solidity built-in `bytes.concat` or (equivalent)
*`abi.encodePacked`.
=======
* For fixed bytes types, we recommend using the solidity built-in `bytes.concat` or (equivalent)
*`abi.encodePacked`.
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Arr00 <[email protected]>
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.
Looks good.
I initially thought about this function to be in Arrays
mimicking the Javascript's API for flatten. If you guys think this is a good approach, then it may be worth exploring.
Javascript's flatten is "Array of Array => Array", which makes sens in the case of javascript because Arrays can contain multiple different types. IMO, Buffer or Strings would be a better match to solidity's bytes than Array. In javascript, Array.flat does NOT merge strings or buffer, which would mean that this concat function (equivalent to Also IMO, this "bytes concatenate" function is more bytes specific then it is array specific. I guess that can be disputed, but I'm not sure it would make sens to have a similar function for arrays of uint256, or arrays of bytes32. It would make sens to have something similar for arrays of strings, but I would put that in Strings.sol, not in Array.sol. |
Mentionned in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5680/files#diff-0399261b2cd954212dda0147b86044380b879ab21e1d1b89c564cb48f3552216R217
See #5680
PR Checklist
npx changeset add
)Summary by Sourcery
Introduce a new concat function in the Bytes library to merge multiple byte buffers into a single bytes value, with comprehensive tests and documentation.
New Features:
Documentation:
Tests:
Chores: