-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Extended packing and extracting library for value types #5056
Conversation
|
Name | Type |
---|---|
openzeppelin-solidity | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
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.
I like the approach and how it looks. However, the Packing.sol
file itself is already 437 kb (around 25% the current size of the library). Let alone the file, I guess the compilation artifacts we distribute will grow as well so probably more.
In a past design, I was suggesting the following size combinations I thought were enough:
const BYTES_PACK_SIZES = {
32: [
['uint128', 'uint128'],
['uint192', 'uint64'],
['uint64', 'uint64', 'uint64', 'uint64'],
],
24: [
['uint96', 'uint96'],
['uint144', 'uint48'],
['uint48', 'uint48', 'uint48', 'uint48'],
],
16: [
['uint64', 'uint64'],
['uint96', 'uint32'],
['uint32', 'uint32', 'uint32', 'uint32'],
],
12: [
['uint48', 'uint48'],
['uint72', 'uint24'],
['uint24', 'uint24', 'uint24', 'uint24'],
],
8: [
['uint32', 'uint32'],
['uint48', 'uint16'],
['uint16', 'uint16', 'uint16', 'uint16'],
],
4: [
['uint16', 'uint16'],
['uint24', 'uint8'],
['uint8', 'uint8', 'uint8', 'uint8'],
],
2: [['uint8', 'uint8']],
};
The idea is to split the most common byte types (2,4,8,12,16,24,32) in 4 (equal) and 2 (unequal) chunks. I realized pairs like [uint48,uint16]
and [uint16,uint48]
were redundant.
The current design feels the right approach but we can reasonably shave off a lot of redundancy from duplicated pack functions (eg. bytes3 | bytes4
vs bytes4 | bytes3
), types such as PackedBytes1
that aren't needed in my opinion, or even extremely specific uses like bytes 2 | bytes27
that aren't worth putting in the library.
What do you think of starting with selecting the combination of types we think are useful and keep the product of them? My list is based on the following?:
- Bytes (32, 16, 8, 4, 2, 1) as starting point. Users may be able to compose them
- Add extra common use cases like ERC4337 nonce packing (
bytes24 | bytes8
), in that case 24 is missing - 12 and 6 to split 24 (we can get rid of these)
Additionally, we can add 20 and 12 for addresses.
With this setup, it's possible that we can reduce this file size substantially while keeping enough usage freedom.
I checked, and the About removing some variants to reduce size:
Something that we never considered, but that may be an alternative is to:
IMO next steps are
|
Proposal:
(that is equivalent to saying pack X+Y<>Z if and only if all X, Y and Z are included types)
This result in a file of 1165 lines (41k). |
I'm not a big fan of the reliance on user defined types in this library. I understand that overloaded function names doesn't work, but it seems very uncomfortable for users to have to wrap and unwrap values to use the library. Have you considered disambiguating by naming convention? For example, |
I think we liked the idea of having the same name when possible, and deducing the right version from the input parameters. We also think that this will possibly be used with both I guess that is not set in stone, we could replace the UDVT with I prefer the current way, but its not a very strong opinion. |
I think the benefit of having the same name may be lost if it requires a high level of verbosity like this: Packing.pack(left.asPackedBytes2(), right.asPackedBytes2()).extract2(0).asUint16() IMO simpler usage should be prioritized: uint16(Packing.pack_2_2(left, right).extract_4_2(0)) The cast is not ideal. Perhaps |
But then if you want to extract a uint from a bytes, or vice versa, that is 4 functions you need ... Usecase: Packing.pack(left.asPackedBytes2(), right.asPackedBytes2()).extract2(0).asUint16() This is testing code, I don't expect that to ever be done in production. IMO productions cases are: // somewhere in contract X
somevariable = Packing.pack(x.asPackedBytes16(), y.asPackedBytes16()).asBytes32();
// somewhere in another contract (or at least another function)
parsed = somevariable.asPackedBytes32().extract16(0).asUint128(); would we prefer ? // somewhere in contract X
somevariable = Packing.pack_16_16(bytes16(x), bytes16(y));
// somewhere in another contract (or at least another function)
parsed = uint128(somevariable.extract_32_16(0)); I'm honestly not sure. |
Why do you need the cast there? |
X and y may be uint128 |
@Amxx @ernestognw How do you feel about the new API? |
Yeah this is a great idea but I share the concerns with dependencies. I've seen people modifying our procedurally generation scripts for custom types. I don't remember the contest but I think it was
I do prefer to distribute a single and "small enough" single library in a Solidity file. I like the current API since I felt initially that we were creating UDVTs for types already defined in the language. I couldn't formulate a concern from that initially becuase I tend to prefer verbosity. Reading the current API is less clear in my opinion, especially considering @Amxx example:
I think the second is clearer but honestly it's a matter of taste. I'd be ok shipping any of both
I also strongly agree with this. Providing the casting variants would be too much in my opinion |
We could replicate the second syntax using the current library if we add (in another PR, let's not do that now) a Cast library that provide all the asXXX primitive you would need library Cast {
function asUint256(bytes32 self) internal pure returns (uint256) { return uint256(self); }
function asBytes32(uint256 self) internal pure returns (bytes32) { return bytes32(self); }
function asAddress(bytes20 self) internal pure returns (address) { return address(self); }
// ...
} |
) internal pure returns (bytes${outer} result) { | ||
bytes${inner} oldValue = extract_${outer}_${inner}(self, offset); | ||
assembly ("memory-safe") { | ||
result := xor(self, shr(mul(8, offset), xor(oldValue, value))) |
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.
The double xor
trick might require an explanation somewhere, but I get that we're avoiding comments on purpose.
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.
For the record:
- the "inner" xor (
xor(oldValue, value)
) computes the "difference" between the old and the new values. - then we shift this difference to place it a the right location
- then the "outer" xor applies this difference to self
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.
LGTM. I added a small guide in the utilities docs and an usage example.
Looks nice 👍 |
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 great! are there any packings use cases in 5.1 that we should update to use this lib?
Alright, thanks everyone for your input. I'm happy with the result and glad we took the time to iterate it before making a release 🙌🏻 |
Fixes #5053
Alternative to #5051
Missing tests for the
replace
functions.PR Checklist
npx changeset add
)