Add Packing library #4992
Add Packing library #4992
Conversation
🦋 Changeset detectedLatest commit: 39add0f 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 |
| * @dev Helper library packing and unpacking multiple values into bytes32 | ||
| */ | ||
| library Packing { | ||
| type Uint128x2 is bytes32; |
There was a problem hiding this comment.
For now we only have (uint128, uint128) <> bytes32, but we can imagine having other types, such as uint64x4
Everything here is designed to scale to other types.
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
Neat! I left only one comment regarding the high and low functions in the context of other types. Also, how would we name other types like uint64|uint64|uint128?
| /// @dev Split a Uint128x2 into two uint128 | ||
| function split(Uint128x2 self) internal pure returns (uint128, uint128) { | ||
| return (high(self), low(self)); | ||
| } | ||
|
|
||
| /// @dev Get the first element of a Uint128x2 (high part) | ||
| function high(Uint128x2 self) internal pure returns (uint128) { | ||
| return uint128(bytes16(Uint128x2.unwrap(self))); | ||
| } | ||
|
|
||
| /// @dev Get the second element of a Uint128x2 (low part) | ||
| function low(Uint128x2 self) internal pure returns (uint128) { | ||
| return uint128(uint256(Uint128x2.unwrap(self))); | ||
| } |
There was a problem hiding this comment.
high and low don't make much sense in the context of another type like Uint64x4, what do you think of only having a single split() function?:
| /// @dev Split a Uint128x2 into two uint128 | |
| function split(Uint128x2 self) internal pure returns (uint128, uint128) { | |
| return (high(self), low(self)); | |
| } | |
| /// @dev Get the first element of a Uint128x2 (high part) | |
| function high(Uint128x2 self) internal pure returns (uint128) { | |
| return uint128(bytes16(Uint128x2.unwrap(self))); | |
| } | |
| /// @dev Get the second element of a Uint128x2 (low part) | |
| function low(Uint128x2 self) internal pure returns (uint128) { | |
| return uint128(uint256(Uint128x2.unwrap(self))); | |
| } | |
| /// @dev Split a Uint128x2 into two uint128 | |
| function split(Uint128x2 self) internal pure returns (uint128, uint128) { | |
| return ( | |
| uint128(bytes16(Uint128x2.unwrap(self))), | |
| uint128(uint256(Uint128x2.unwrap(self))) | |
| ) | |
| } |
There was a problem hiding this comment.
There are places where doing a full split, and keeping only part of the result is a bit more expensive, and also less clean in terms of code.
I would keep these two function, but we can rename them.
I considered first() and second(), which may scale better
There was a problem hiding this comment.
Right, also I realized it would be easy to reach a Stack too deep error if we pack too many values, so I'd be in favor of having single getters.
Agree with first and second, but the distinction with high and low is that the former doesn't express the order (i.e. first ltr or rtl?). Shall we make it explicit? Maybe by exposing Edit: Nah, let's keep first() and firstRtl()first and second and be explicit in the Natspec
Just realized that if we have an |
2 participants
Will be usefull for processing "packed" values such as
accountGasLimitsandgasFeesas defined in ERC-4337PR Checklist
npx changeset add)