Use packed stack-based version of Slot0 structure #613
Conversation
|
Btw in general, I do really like this change and it would be great to get tests I think! cc @hensha256 |
0xVolosnikov
force-pushed
the
isomorphic-slot0
branch
from
acaec93
to
24cf25e
Compare
0xVolosnikov
force-pushed
the
isomorphic-slot0
branch
from
b457246
to
6707a51
Compare
0xVolosnikov
force-pushed
the
isomorphic-slot0
branch
from
311c92a
to
6b919a0
Compare
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
src/types/Slot0.sol
Outdated
| uint24 protocolFee; | ||
| // used for the lp fee, either static at initialize or dynamic via hook | ||
| uint24 lpFee; | ||
| */ |
There was a problem hiding this comment.
Could you maybe just also add a comment that we dont use a struct so that we get the benefits of a packed structure when we reference slot0 in memory/stack
src/types/Slot0.sol
Outdated
| using Slot0Library for Slot0 global; | ||
|
|
||
| library Slot0Library { | ||
| uint160 private constant UINT160_MASK = 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; |
There was a problem hiding this comment.
why is this not just 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ?
There was a problem hiding this comment.
Compiler treats it as an incorrectly formatted address literal and is really angry about it
There was a problem hiding this comment.
could just change it to type(uint160).max? That feels more reliable anyway as you dont need to check you have the right number of Fs
There was a problem hiding this comment.
could just change it to
type(uint160).max?
Only direct number constants can be used in assembly. We can replace it with type(uin160).max but it will be a bit messier
There was a problem hiding this comment.
Generally really like this! Any chance of some tests?
Particularly a fuzz test that takes in the 4 arguments, and calls set on all 4, and then get on all 4, and check the extracted values are the same as the inputs.
Plus a test that UINT160_MASK == type(uint160).max in case anyone ever edits it 😏
src/types/Slot0.sol
Outdated
|
|
||
| library Slot0Library { | ||
| uint160 internal constant UINT160_MASK = 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; | ||
| uint24 internal constant INT24_MASK = 0xFFFFFF; |
There was a problem hiding this comment.
INT24_MASK can be removed since it is equal to UINT24_MASK
|
Added tests |
|
@snreynolds anything else you wanna see before we merge? |
| state.feeGrowthGlobalX128 = zeroForOne ? self.feeGrowthGlobal0X128 : self.feeGrowthGlobal1X128; | ||
| state.liquidity = liquidityStart; | ||
|
|
||
| // if the beforeSwap hook returned a valid fee override, use that as the LP fee, otherwise load from storage | ||
| slot0Start.lpFee = | ||
| params.lpFeeOverride.isOverride() ? params.lpFeeOverride.removeOverrideAndValidate() : slot0Start.lpFee; | ||
| { |
This pull request does not relate to any issue. In my opinion, it is more convenient to discuss this change if you have an already written implementation in a pull request.
Description of changes
The slot0 structure by design fits into one storage slot. However, when using the memory version of this structure, each field occupies a separate memory slot. In addition, the memory structure takes up more space on the solidity "stack".
The changes I propose allows to read this structure onto the stack and operate with the fields of this structure directly on the stack. As a result, the amount of memory used is reduced, which a little bit reduces gas costs and bytecode size.
Since the changes are more or less significant, I would first like to hear the opinion of the community and maintainers regarding the proposal to use a structure on the stack before adding tests and so on.