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

V6 replace uni v2 router and add polygon #300

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

charlesjhongc
Copy link
Contributor

@charlesjhongc charlesjhongc commented Nov 8, 2023

  • Replace old Uniswap v2 router with SwapRouter02
  • Add polygon network config

Copy link

github-actions bot commented Nov 8, 2023

Changes to gas cost

Generated at commit: cab522ce789aadfb78cde639da4be82d2605ec75, compared to commit: 4b550124a71a5853f941631d55cd4e2871bc0043

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
SmartOrderStrategy approveTokens -466,779 ✅ -68.67%
LimitOrderSwap fillLimitOrder
fillLimitOrderFullOrKill
+81,806,805,130,593,220 ❌
-13,505 ✅
+21.05%
-15.07%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
SmartOrderStrategy 1,172,950 (0) approveTokens
executeStrategy
34,188 (-253,940)
709 (0)
-88.13%
0.00%
212,937 (-466,779)
80,366 (+1,808)
-68.67%
+2.30%
233,101 (-511,880)
45,143 (+2,410)
-68.71%
+5.64%
373,430 (-371,551)
493,162 (+6,027)
-49.87%
+1.24%
25 (+11)
14 (0)
LimitOrderSwap 2,513,778 (0) cancelOrder
fillLimitOrder
fillLimitOrderFullOrKill
5,557 (0)
5,990 (0)
18,100 (0)
0.00%
0.00%
0.00%
17,439 (+1,612)
470,389,129,500,982,460 (+81,806,805,130,593,220)
76,111 (-13,505)
+10.19%
+21.05%
-15.07%
17,592 (+11,436)
168,529 (+49,783)
58,920 (-45,585)
+185.77%
+41.92%
-43.62%
29,061 (0)
8,937,393,460,516,586,000 (0)
168,506 (0)
0.00%
0.00%
0.00%
6 (-1)
19 (-4)
4 (-3)
CoordinatedTaker 2,201,199 (0) submitLimitOrderFill 1,328 (0) 0.00% 107,157 (+13,975) +15.00% 96,492 (+58,240) +152.25% 224,999 (0) 0.00% 6 (-1)
AllowanceTarget 570,375 (0) spendFromUserTo 11,608 (0) 0.00% 21,378 (-1,837) -7.91% 21,378 (-5,511) -20.50% 31,148 (0) 0.00% 2 (-1)
UniAgent 1,412,993 (0) approveAndSwap
swap
9,219 (0)
88,963 (0)
0.00%
0.00%
102,963 (+1,666)
151,775 (+8)
+1.64%
+0.01%
127,755 (+2,500)
148,765 (-1,553)
+2.00%
-1.03%
171,917 (+2,500)
211,668 (0)
+1.48%
0.00%
3 (0)
12 (0)

require(path.length >= 2, "UniswapV2Library: INVALID_PATH");
amounts = new uint256[](path.length);
amounts[0] = amountIn;
for (uint256 i; i < path.length - 1; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace the suffix increment with the prefix one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 3652669

}

// performs chained getAmountOut calculations on any number of pairs
function getAmountsOut(uint256 amountIn, address[] memory path) internal view returns (uint256[] memory amounts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use calldata to store the path is better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We create path in memory in tests so can't use calldata here. Also, these utils are for test only so we won't spend too much effort on optimizing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see.

Copy link
Contributor

@alex0207s alex0207s left a comment

Choose a reason for hiding this comment

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

Leave some comments for the better practices, overall LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

This strategy is for testing purposes, there is no need to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This strategy is for testing purposes, there is no need to keep it.

"ARBITRUM_L1_GATEWAY_ROUTER_ADDR": "0x0000000000000000000000000000000000000000",
"ARBITRUM_L1_BRIDGE_ADDR": "0x0000000000000000000000000000000000000000",
"OPTIMISM_L1_STANDARD_BRIDGE_ADDR": "0x0000000000000000000000000000000000000000"
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x4648a43B2C14Da09FdF82B161150d3F634f40491"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x4648a43B2C14Da09FdF82B161150d3F634f40491"
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x4C60051384bd2d3C01bfc845Cf5F4b44bcbE9de5"
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS_V1_2": "0xeC8B0F7Ffe3ae75d7FfAb09429e3675bb63503e4"

Addresses should be these according to latest code: https://github.com/Uniswap/universal-router/blob/main/deploy-addresses/arbitrum.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 22e1a59

"CRV_ADDRESS": "0x172370d5Cd63279eFa6d502DAB29171933a610AF",
"TUSD_ADDRESS": "0x2e1AD108fF1D8C782fcBbB89AAd783aC49586756",
"DAI_ADDRESS": "0x8f3Cf7ad23Cd3CaDbD9735AFf958023239c6A063",
"LON_ADDRESS": "0x6f7C932e7684666C9fd1d44527765433e01fF61d",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"LON_ADDRESS": "0x6f7C932e7684666C9fd1d44527765433e01fF61d",
"MKR_ADDRESS": "0x6f7C932e7684666C9fd1d44527765433e01fF61d",
"LON_ADDRESS": "0x000000000000000000000000000000000000000",

This address belongs to MAKER on Polygon: https://polygonscan.com/address/0x6f7C932e7684666C9fd1d44527765433e01fF61d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but we don't have LON on polygon.

"UNISWAP_V3_QUOTER_ADDRESS": "0xb27308f9F90D607463bb33eA1BeBb41C27CE5AB6",
"UNISWAP_PERMIT2_ADDRESS": "0x000000000022d473030f116ddee9f6b43ac78ba3",
"UNISWAP_SWAP_ROUTER_02_ADDRESS": "0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45",
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD"
"UNISWAP_UNIVERSAL_ROUTER_ADDRESS": "0x4C60051384bd2d3C01bfc845Cf5F4b44bcbE9de5"

Universal router is 0x4C60051384bd2d3C01bfc845Cf5F4b44bcbE9de5 according to latest code: https://github.com/Uniswap/universal-router/blob/main/deploy-addresses/polygon.json
However 0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD is still a workable address at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 22e1a59

@charlesjhongc
Copy link
Contributor Author

Thank you @108356037 for carefully checking the addresses. I found that Uniswap no longer use same address across multiple networks so can't assume it's all the same anymore.

@charlesjhongc charlesjhongc merged commit d894011 into master Nov 9, 2023
2 checks passed
@charlesjhongc charlesjhongc deleted the v6-replace-uni-v2-router-and-add-polygon branch November 9, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants