feat: whitelist 0x routers #810
Conversation
WalkthroughThe changes involve updates to the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Coverage ReportLine Coverage: 75.15% (1576 / 2097 lines) |
|
One question: shall we delete previous approved router contracts after we successfully update 0x api from v1 to v2? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
config/dexs.json (1)
Additional Addresses Added Across Multiple Networks
The primary address (
0x0000000000001ff3684f28c67538d4d072c22734) has been successfully added to the whitelist across multiple networks. However, several additional addresses have been included that were not outlined in the PR objectives. These additions span numerous networks, which may require further clarification to ensure they align with the project's goals.Could you please provide insights into the purpose of these additional addresses and confirm whether their inclusion is intentional? Understanding the rationale behind these changes will help maintain the project's integrity and ensure all modifications are purposeful.
Analysis chain
Line range hint
1-657: Summary of changes and request for clarificationThe primary objective of adding the new AllowanceHolder Router (
0x0000000000001ff3684f28c67538d4d072c22734) to the whitelist has been achieved across multiple networks. However, several additional addresses were added that weren't mentioned in the PR objectives:
0x216b4b4ba9f3e719726886d34a177484278bfcae(added to arbitrum and optimism)0x0000000000005e88410ccdfade4a5efae4b49562(added to avalanche)0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959(added to blast)0x111111125421ca6dc452d289314280a0f8842a65(added to polygon)0x93aaae79a53759cd164340e4c8766e4db5331cd7(added to base)Could you please provide clarification on the purpose of these additional addresses and explain why different addresses were added to different networks? This information will help us understand the full scope of the changes and ensure that they align with the project's objectives.
To get a complete picture of the changes, let's run a script to show all new addresses added across all networks:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Show all new addresses added across all networks echo "New addresses added to each network:" grep -n '^[ ]*"0x' config/dexs.json | sort -n | uniq -c | awk '$1 == 1 {print $0}' | sed 's/^[ \t]*//' | sed 's/:/: /'Length of output: 470
Script:
#!/bin/bash # Extract all addresses from config/dexs.json and identify additional addresses not part of the primary objective. # Define the primary address mentioned in the PR objectives PRIMARY_ADDRESS="0x0000000000001ff3684f28c67538d4d072c22734" # Use jq to extract all unique addresses across all networks jq -r 'to_entries[].value[]' config/dexs.json | sort | uniq > all_addresses.txt # Check if the primary address exists and list additional addresses if grep -qi "$PRIMARY_ADDRESS" all_addresses.txt; then echo "Primary address ($PRIMARY_ADDRESS) is present." echo "Additional addresses added:" grep -vi "$PRIMARY_ADDRESS" all_addresses.txt else echo "Primary address ($PRIMARY_ADDRESS) is missing." fiLength of output: 10290
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- config/dexs.json (10 hunks)
Additional comments not posted (7)
config/dexs.json (7)
608-609: New addresses added to base network - clarification needed.The address
0x0000000000001ff3684f28c67538d4d072c22734has been added as expected. However, an additional address0x93aaae79a53759cd164340e4c8766e4db5331cd7was also added, which wasn't mentioned in the PR objectives.Could you please clarify the purpose of adding
0x93aaae79a53759cd164340e4c8766e4db5331cd7? Let's check if this address is present in other networks:#!/bin/bash # Check for the additional base address across all networks echo "Networks containing the additional base address:" grep -l '"0x93aaae79a53759cd164340e4c8766e4db5331cd7"' config/dexs.json | xargs -I {} grep -B1 '"0x93aaae79a53759cd164340e4c8766e4db5331cd7"' {} | grep -v "0x93aaae79a53759cd164340e4c8766e4db5331cd7" | sed 's/[":,]//g'
134-135: Different address added to avalanche network - clarification needed.A new address
0x0000000000005e88410ccdfade4a5efae4b49562has been added to the avalanche network. This address is different from the one added to mainnet and arbitrum, and it wasn't mentioned in the PR objectives.Could you please explain the reason for adding this different address to the avalanche network? Let's check if this address is present in other networks:
#!/bin/bash # Check for the new avalanche address across all networks echo "Networks containing the new avalanche address:" grep -l '"0x0000000000005e88410ccdfade4a5efae4b49562"' config/dexs.json | xargs -I {} grep -B1 '"0x0000000000005e88410ccdfade4a5efae4b49562"' {} | grep -v "0x0000000000005e88410ccdfade4a5efae4b49562" | sed 's/[":,]//g'
145-146: New addresses added to blast network - clarification needed.The address
0x0000000000001ff3684f28c67538d4d072c22734has been added as expected. However, an additional address0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959was also added, which wasn't mentioned in the PR objectives.Could you please clarify the purpose of adding
0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959? Let's check if this address is present in other networks:#!/bin/bash # Check for the additional blast address across all networks echo "Networks containing the additional blast address:" grep -l '"0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959"' config/dexs.json | xargs -I {} grep -B1 '"0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959"' {} | grep -v "0xcdbcd51a5e8728e0af4895ce5771b7d17ff71959" | sed 's/[":,]//g'
534-535: New addresses added to polygon network - clarification needed.The address
0x0000000000001ff3684f28c67538d4d072c22734has been added as expected. However, an additional address0x111111125421ca6dc452d289314280a0f8842a65was also added, which wasn't mentioned in the PR objectives.Could you please clarify the purpose of adding
0x111111125421ca6dc452d289314280a0f8842a65? Let's check if this address is present in other networks:
38-39: LGTM: New AllowanceHolder Router added to mainnet.The new address
0x0000000000001ff3684f28c67538d4d072c22734has been successfully added to the mainnet network. This change aligns with the PR objectives.Let's verify if this address is consistently added to other major networks:
Verification successful
Verified: New AllowanceHolder Router added across multiple networks.
The new address
0x0000000000001ff3684f28c67538d4d072c22734has been successfully added to the following networks: mainnet, arbitrum, blast, bsc, optimism, polygon, and base. This change aligns with the PR objectives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the new address across major networks echo "Networks containing the new address:" grep -l '"0x0000000000001ff3684f28c67538d4d072c22734"' config/dexs.json | xargs -I {} grep -B1 '"0x0000000000001ff3684f28c67538d4d072c22734"' {} | grep -v "0x0000000000001ff3684f28c67538d4d072c22734" | sed 's/[":,]//g'Length of output: 472
Script:
#!/bin/bash # Check for the new address across major networks ADDRESS="0x0000000000001ff3684f28c67538d4d072c22734" echo "Networks containing the new address $ADDRESS:" grep -l "\"$ADDRESS\"" config/dexs.json | xargs -I {} grep -B1 "\"$ADDRESS\"" {} | grep -v "$ADDRESS" | sed 's/[":,]//g'Length of output: 791
Script:
#!/bin/bash # Find networks containing the new address ADDRESS="0x0000000000001ff3684f28c67538d4d072c22734" echo "Networks containing the new address $ADDRESS:" jq --arg addr "$ADDRESS" 'to_entries | map(select(.value | index($addr)) | .key)' config/dexs.jsonLength of output: 441
77-78: New addresses added to arbitrum network - clarification needed.The address
0x0000000000001ff3684f28c67538d4d072c22734has been added as expected. However, an additional address0x216b4b4ba9f3e719726886d34a177484278bfcaewas also added, which wasn't mentioned in the PR objectives.Could you please clarify the purpose of adding
0x216b4b4ba9f3e719726886d34a177484278bfcae? Let's check if this address is present in other networks:
490-491: New addresses added to optimism network - consistent with arbitrum.The addresses added to the optimism network are the same as those added to the arbitrum network:
0x216b4b4ba9f3e719726886d34a177484278bfcae0x0000000000001ff3684f28c67538d4d072c22734The second address is consistent with the PR objectives. However, the purpose of the first address is still unclear.
As we've seen this pattern before, let's check if these two addresses are consistently added together across other networks:
|
Please also add signature |
|
The whitelisting has been synced to the chains supported by 0x. |
|
We need a sc core dev approve |
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-9741
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)