Skip to content
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

Merged
merged 5 commits into from
Jan 16, 2024
Merged

gas optimization: leave one wei feature #303

merged 5 commits into from
Jan 16, 2024

Conversation

alex0207s
Copy link
Contributor

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.

  • Implement the logic for leaving 1 wei in GenericSwap.sol
  • Implement the logic for leaving 1 wei in SmartOrderStrategy.sol

Copy link

github-actions bot commented Dec 5, 2023

Changes to gas cost

Generated at commit: e014f31eceea2818d2f0861b8e0b8f1ea2f92697, compared to commit: 30182962c00a4fff72b4b11b72989fd64b00d46e

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
GenericSwap executeSwap
executeSwapWithSig
+27,482 ❌
+26,454 ❌
+53.77%
+22.97%
LimitOrderSwap fillLimitOrderFullOrKill
fillLimitOrderGroup
+17,847 ❌
+44,010 ❌
+19.91%
+22.80%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
GenericSwap 1,648,811 (0) executeSwap
executeSwapWithSig
678 (0)
4,021 (0)
0.00%
0.00%
78,593 (+27,482)
141,621 (+26,454)
+53.77%
+22.97%
69,421 (+46,410)
146,087 (+25,904)
+201.69%
+21.55%
229,258 (+45,802)
270,290 (+54,008)
+24.97%
+24.97%
12 (+2)
4 (0)
LimitOrderSwap 2,513,778 (0) cancelOrder
fillLimitOrder
fillLimitOrderFullOrKill
fillLimitOrderGroup
5,557 (0)
5,990 (0)
18,100 (0)
194,749 (+38,949)
0.00%
0.00%
0.00%
+25.00%
17,027 (+1,200)
388,582,324,370,404,700 (+15,232)
107,463 (+17,847)
237,063 (+44,010)
+7.58%
+0.00%
+19.91%
+22.80%
6,156 (0)
139,615 (+20,869)
127,205 (+22,700)
237,546 (+42,600)
0.00%
+17.57%
+21.72%
+21.85%
31,861 (+2,800)
8,937,393,460,516,606,000 (+19,456)
191,206 (+22,700)
291,787 (+58,357)
+9.63%
+0.00%
+13.47%
+25.00%
7 (0)
23 (0)
7 (0)
6 (0)
SmartOrderStrategy 1,178,556 (+5,606) approveTokens
executeStrategy
34,188 (0)
709 (0)
0.00%
0.00%
152,602 (-4,737)
101,399 (+16,680)
-3.01%
+19.69%
233,101 (0)
111,081 (+22,272)
0.00%
+25.08%
257,601 (0)
575,041 (+79,635)
0.00%
+16.07%
26 (+1)
19 (+2)
CoordinatedTaker 2,201,199 (0) submitLimitOrderFill 1,328 (0) 0.00% 108,596 (+15,414) +16.54% 38,252 (0) 0.00% 267,599 (+42,600) +18.93% 7 (0)
UniAgent 1,412,993 (0) approveAndSwap
swap
9,219 (0)
88,963 (0)
0.00%
0.00%
118,097 (+15,134)
169,992 (+18,217)
+14.70%
+12.00%
150,455 (+22,700)
172,168 (+23,403)
+17.77%
+15.73%
194,617 (+22,700)
231,568 (+19,900)
+13.20%
+9.40%
3 (0)
12 (0)
RFQ 2,287,309 (0) fillRFQ
fillRFQWithSig
2,134 (0)
2,741 (0)
0.00%
0.00%
107,125 (+12,102)
83,240 (+6,633)
+12.74%
+8.66%
125,216 (+18,611)
46,495 (0)
+17.46%
0.00%
212,334 (+21,314)
200,485 (+19,900)
+11.16%
+11.02%
23 (0)
3 (0)
AllowanceTarget 570,375 (0) spendFromUserTo 11,608 (0) 0.00% 25,810 (+2,595) +11.18% 26,889 (0) 0.00% 38,934 (+7,786) +25.00% 3 (0)

@alex0207s alex0207s requested a review from 108356037 December 5, 2023 07:17
Copy link
Contributor

@charlesjhongc charlesjhongc left a 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.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gas or GS?

Copy link
Contributor Author

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.

gsData.takerToken,
gsData.takerTokenAmount,
gsData.makerToken,
gsData.makerTokenAmount - 1
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@alex0207s alex0207s changed the title Leave one wei gas optimization: leave one wei feature Jan 11, 2024
Copy link
Contributor

@charlesjhongc charlesjhongc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alex0207s alex0207s merged commit fd26a14 into master Jan 16, 2024
2 checks passed
@charlesjhongc charlesjhongc deleted the leave-one-wei branch January 17, 2024 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants