Merge hooks config with pool config #659
Conversation
|
I agree with you. I will split it. By the gas and bytecode size, it will be the same |
There was a problem hiding this comment.
Thanks for pushing this @elshan-eth, great job.
Some general comments:
- I think I prefer this approach, even if the other one saves a bit more gas. The reason is that having the
anyHookflag is kinda duplicating the information that we have in the other flags, which adds some extra complexity to the reader. - About this approach: if I'm not mistaken, in the current
mainwe are forced to do 2 storage reads for pool config and hook config, even if we don't need to call any hooks. By merging the flags, we can have only one storage read, and defer reading the hook address only in case we actually need it. I think this optimization is good enough (i.e. have all flags in one slot, and hook address in another one). If we have to read the hook address from storage twice because we havebeforeandafterhooks that's fine; the second one will be warm (100 gas).
Personally I'd remove the cache, and just read the hook address from storage only when needed. That should help us keep the hook logic pretty well self-contained.
Anyways, I really welcome the effort in this exploration. There are no clear good answers to this; just a big pile of trade-offs to choose from.
| hookFlags.shouldCallAfterRemoveLiquidity | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this refactor needed for this PR? If not, I'd suggest opening a separate PR just with the refactor. Let's try to keep this one minimal, with changes related only to the optimization itself.
There was a problem hiding this comment.
Yes, I don’t quite understand what you mean. here, I changed variable witch we set (hooksConfig->poolConfigBits) + moved these to the zone where poolConfigBits set other parameters
There was a problem hiding this comment.
Yes, I don’t quite understand what you mean. here, I changed the variable which we set (hooksConfig->poolConfigBits) + moved these to the zone where poolConfigBits set other parameters
There was a problem hiding this comment.
Same as above. This doesn't seem to be strictly related with the optimization; it can be a PR in it's own right, which will make it easier to review.
Hooks config lib needs to depend on pool config lib anyways right?
There was a problem hiding this comment.
Do you mean to move all constants to Pool ConfigLib? And do it in a different PR? Sounds a little strange to me, but I can do it
There was a problem hiding this comment.
I've looked at this so many times I might be too familiar - but looks great now. The main thing I wanted to see was moving the "shouldCall" checks outside of the library, to save loading all the parameters up and making the call, just to immediately return. Looks much cleaner now!
Description
This PR is about merging hooks config and pool config. To preserve the functions onComputeDynamicSwapFee, onBeforeSwap, and others in PoolConfigLib, I decided to create a Cache library. These functions use hooksContract, and Cache allows loading variables from storage once and subsequently using them from memory. Another approach would be to remove these functions from PoolConfigLib and implement all the logic directly in the places where they are used.
Also, I made a mistake with PoolConfigLib.t.sol in the previous PR, and this PR also fixes these tests.
Alternative approach #671.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution
Closes #656