-
Notifications
You must be signed in to change notification settings - Fork 23
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
gas optimization: leave one wei feature #303
Conversation
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
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.
Overall I think both ways are fine as long as cases with and without 1wei in contracts are all covered in tests. However, cases with amount replacement are more important to include the 1wei context since the way it affects the ratio calculation.
test/forkMainnet/GenericSwap.t.sol
Outdated
@@ -156,16 +157,17 @@ contract GenericSwapTest is Test, Tokens, BalanceUtil, Permit2Helper, SigHelper | |||
Snapshot memory makerMakerToken = BalanceSnapshot.take({ owner: address(mockStrategy), token: gsData.makerToken }); | |||
|
|||
uint256 actualOutput = 900; | |||
uint256 realChangedInGas = actualOutput - 1; |
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.
Gas or GS?
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.
It's a typo, it should be GS
here.
test/forkMainnet/GenericSwap.t.sol
Outdated
gsData.takerToken, | ||
gsData.takerTokenAmount, | ||
gsData.makerToken, | ||
gsData.makerTokenAmount - 1 |
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 think we can have expectedOut = gsData.makerTokenAmount - 1
for these assertion.
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.
Good idea, but maybe call it realChangedInGS
for consistency.
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
For gas optimization purposes, we retain 1 wei of each token in our contract(GS and SOS). This will impact the initial swap of every token, as 1 wei will be left in our contract.
GenericSwap.sol
SmartOrderStrategy.sol