-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: Add Polygon support to New CCTP Router Contracts #1617
Conversation
- Clear CCTP_ROUTER_ADDRESS for future update - Add Polygon to CCTP_SUPPORTED_CHAIN_IDS - Define median time for Polygon in MEDIAN_TIME_CCTP
WalkthroughThe SDK router's configuration is undergoing significant updates. The Changes
Poem
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 X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/sdk-router/src/constants/addresses.ts (1 hunks)
- packages/sdk-router/src/constants/chainIds.ts (1 hunks)
- packages/sdk-router/src/constants/medianTime.ts (1 hunks)
Additional comments: 4
packages/sdk-router/src/constants/addresses.ts (1)
- 44-45: The
CCTP_ROUTER_ADDRESS
has been cleared and is now an empty string. Ensure that this placeholder does not lead to runtime errors in the system where a valid contract address is expected. It might be necessary to check if there are any parts of the code that could attempt to interact with the contract at this address before the update is completed.packages/sdk-router/src/constants/chainIds.ts (2)
39-42: The addition of
SupportedChainId.POLYGON
to theCCTP_SUPPORTED_CHAIN_IDS
array is consistent with the PR objectives to support the Polygon network. Ensure that the comment "Circle domain 7" is accurate and that the domain number aligns with the official documentation or configuration for the Polygon network within the CCTP protocol.39-43: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [26-30]
The dynamic generation of
SUPPORTED_CHAIN_IDS
from theSupportedChainId
enum values is a good practice for maintainability and reducing manual errors. It ensures that any updates to the enum are automatically reflected in the supported chain IDs.packages/sdk-router/src/constants/medianTime.ts (1)
- 36-40: The addition of the
SupportedChainId.POLYGON
entry to theMEDIAN_TIME_CCTP
object with a value of 440 is consistent with the rest of the file and follows the established pattern for defining median times. This change aligns with the PR objectives to support the Polygon network.
const CCTP_ROUTER_ADDRESS = '0xD359bc471554504f683fbd4f6e36848612349DDF' | ||
|
||
// TODO: Update to use new Router contract addresses | ||
const CCTP_ROUTER_ADDRESS = '' | ||
const CCTP_ROUTER_EXCEPTION_MAP: AddressMap = {} | ||
|
||
export const CCTP_ROUTER_ADDRESS_MAP: AddressMap = generateAddressMap( |
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.
💡 NOTE
Codebase verification is a beta feature.
The review of the SynapseCCTPRouter
class indicates that the routerContract
is used for blockchain interactions, which typically require a valid contract address. The current state of CCTP_ROUTER_ADDRESS_MAP
being an empty string could lead to failures when these methods are called. There is no direct evidence of checks for the address being non-empty before these interactions occur. It is important to ensure that the system can handle this case or that there are plans to update the address before any blockchain interactions occur.
- The
SynapseCCTPRouter
class usesthis.routerContract
for blockchain interactions, which may fail if instantiated with an empty string as the contract address. - There is no evidence of checks for non-empty addresses before blockchain interactions in the
SynapseCCTPRouter
class.
Analysis chain
The CCTP_ROUTER_ADDRESS_MAP
is generated with the CCTP_ROUTER_ADDRESS
which is now an empty string. Verify that the system can handle the address map containing empty strings for all chain IDs without any issues.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of `CCTP_ROUTER_ADDRESS_MAP` to ensure it's not used in a way that requires a non-empty string.
rg 'CCTP_ROUTER_ADDRESS_MAP' --files-with-matches | xargs rg 'CCTP_ROUTER_ADDRESS_MAP'
Length of output: 1752
Script:
#!/bin/bash
# Check the implementation details in synapseCCTPRouterSet.ts to see how CCTP_ROUTER_ADDRESS_MAP is used.
rg 'CCTP_ROUTER_ADDRESS_MAP' packages/sdk-router/src/router/synapseCCTPRouterSet.ts
# Check if there are any checks or fallbacks in place when an empty string is encountered in the address map.
rg 'CCTP_ROUTER_ADDRESS_MAP' --files-with-matches | xargs rg 'if.*CCTP_ROUTER_ADDRESS_MAP'
Length of output: 309
Script:
#!/bin/bash
# Check the parent class SynapseCCTPRouter to see how it handles the CCTP_ROUTER_ADDRESS_MAP.
rg 'class SynapseCCTPRouter' --files-with-matches | xargs rg 'SynapseCCTPRouter'
Length of output: 2419
Script:
#!/bin/bash
# Search for the constructor and any methods in SynapseCCTPRouter that use the CCTP_ROUTER_ADDRESS_MAP.
rg 'constructor' packages/sdk-router/src/router/synapseCCTPRouter.ts
rg 'CCTP_ROUTER_ADDRESS_MAP' packages/sdk-router/src/router/synapseCCTPRouter.ts
Length of output: 289
Script:
#!/bin/bash
# Search for any method within SynapseCCTPRouter that uses the 'address' argument and check how it handles it.
rg 'this.routerContract' packages/sdk-router/src/router/synapseCCTPRouter.ts
Length of output: 384
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/sdk-router/src/constants/addresses.ts (1 hunks)
- packages/sdk-router/src/constants/medianTime.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk-router/src/constants/addresses.ts
- packages/sdk-router/src/constants/medianTime.ts
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1617 +/- ##
===================================================
- Coverage 51.47991% 51.36572% -0.11419%
===================================================
Files 366 362 -4
Lines 24765 24639 -126
Branches 284 284
===================================================
- Hits 12749 12656 -93
+ Misses 10792 10763 -29
+ Partials 1224 1220 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/sdk-router/src/constants/addresses.ts (1 hunks)
Additional comments: 1
packages/sdk-router/src/constants/addresses.ts (1)
- 46-46: The previous concerns about
CCTP_ROUTER_ADDRESS
being an empty string and potential issues with theSynapseCCTPRouter
class are no longer valid as the address has been updated to a non-empty string.
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
* update for new tokens (#1623) * feat: Add Polygon support to New CCTP Router Contracts (#1617) * feat: Add Polygon support to SDK Router - Clear CCTP_ROUTER_ADDRESS for future update - Add Polygon to CCTP_SUPPORTED_CHAIN_IDS - Define median time for Polygon in MEDIAN_TIME_CCTP * Add new SynapseCCTpRouter address * Adjust median time for Polygon: 8 minutes * Chore: remove extra line --------- Co-authored-by: ChiTimesChi <[email protected]> * Publish - @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected] * Regenerate agents (#1575) * adding cctp contracts to the explorer UI (#1626) * adding cctp contracts * Add Ethereum CCTP contract --------- Co-authored-by: ChiTimesChi <[email protected]> * Publish - @synapsecns/[email protected] * FE: add polygon native USDC and FRAX (#1627) * Update values in generation script * Update CCTP Router Address * Regenerate bridge map * Add FRAX address on Polygon --------- Co-authored-by: bigboydiamonds <[email protected]> * Publish - @synapsecns/[email protected] * adding frax support on polygon (#1631) * Publish - @synapsecns/[email protected] --------- Co-authored-by: Moses <[email protected]> Co-authored-by: aureliusbtc <[email protected]> Co-authored-by: ChiTimesChi <[email protected]> Co-authored-by: bigboydiamonds <[email protected]>
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Documentation
Bug Fixes