v4-core
v4-core copied to clipboard
Uniswap
Improve readability of totalSwapFee numeric literal
Improving the readability of the maximum value for totalSwapFee is a QoL improvement that makes a big difference for future devs building on v4 hooks.
The visual 1_000_000 strongly communicates the value on first glance, as opposed to 1000000 This readable format is mentioned in the Solidity docs here:
https://docs.soliditylang.org/en/latest/types.html#rational-and-integer-literals
Related Issue
Which issue does this pull request resolve?
Description of changes
might be even better as a constant?
Great point!
All three of the other constant variables are similarly invoked only once or twice, so adding a constant MAX_POOL_SWAP_FEE = 1_000_000 is consistent with the existing structure and improves readability.
I've added the constant and updated the accompanying interface to match, with all tests passing as before since it doesn't change any functionality. Let me know what you think :)
Just one natspec suggestion, and please can you merge in main! Then I'll approve and merge :)
Sorryyyy each time you merge in main you have to check the yarn snapshots in case gas has changed. You did snapshots then merged main so theyre still wrong 😆
Please can you check main is up to date, then run yarn snapshots and push them.
Additionally, I can see that commit 1f4354a is not a signed commit. We can't merge anything that isnt signed. Please can you sign it 🙏
Thank you so much for the guidance!
I have not used snapshots before as I generally work in Foundry-only repos. The Hardhat+Foundry monorepo structure with Yarn threw me for a loop. Everything should be up to date with accurate snapshots now :)
**The unsigned commit was due to using a newer computer without a configured ssh signing key. I rectified that commit with the rebase so hopefully all is set to merge into main now. 🙏
Hi! So sorry for the delay here - I took some time off work. Please could you do the merge conflicts and make sure snapshots are up to date. I'll get this merged for you asap 🙌
My turn for a delay: was travelling. Merge conflicts resolved, added the same constant to the FeeLibrary, yarn snapshots run. LMK if you would like any other changes.
Hi there, appreciate the contribution! Overall I like this change but the contracts have changed quite a bit so I will close and just ask you to re-open this contribution after updating main. Please tag me for a review when it's ready!
