-
Notifications
You must be signed in to change notification settings - Fork 504
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
Optimise actions encoding #234
Conversation
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.
this is super cool, didn't even think to do this/didn't realize it would save so much gas
|
||
uint256 numActions = actions.length; | ||
if (numActions != params.length) revert InputLengthMismatch(); | ||
|
||
for (uint256 actionIndex = 0; actionIndex < numActions; actionIndex++) { | ||
uint256 action = actions[actionIndex]; | ||
uint256 action = uint256(uint8(actions[actionIndex])); |
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.
would this line be incorrect to do uint256(actions[actionIndex])
?
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.
wont compile because actions[actionIndex]
is a bytes1
so you have to cast it via uint8
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.
you always have to cast to a different type of the same size before you change sizes.
e.g. if you have int128 and you want uint256 you have to do uint256(uint128(int128))
so in this case you have ot go to uint8
because its the same size as bytes1
Currently all our actions are well within 1 byte, but are being encoded as an array of 256 bit numbers - very wasteful on calldata. Now theyre packed as 1 byte commands like in the UR