Adding ETH on BNB Chain [SLT-398] #3338
Conversation
Defi-Moses
requested review from
bigboydiamonds,
abtestingalpha,
aureliusbtc and
trajan0x
as code owners
WalkthroughThe changes in this pull request introduce new entries for the 'ETH' token across multiple files, enhancing the bridge functionality and token mappings. Specifically, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
7de1c18
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bf8298fd.sanguine-fe.pages.dev |
| Branch Preview URL: | https://update-tokens-constants.sanguine-fe.pages.dev |
Bundle ReportChanges will increase total bundle size by 270.7kB (0.85%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- packages/rest-api/src/constants/bridgeMap.ts (2 hunks)
- packages/rest-api/src/constants/bridgeable.ts (1 hunks)
- packages/synapse-constants/src/constants/bridgeMap.ts (2 hunks)
- packages/synapse-constants/src/constants/tokens/bridgeable.ts (1 hunks)
- packages/synapse-interface/constants/bridgeMap.ts (2 hunks)
- packages/synapse-interface/constants/tokens/bridgeable.ts (2 hunks)
- packages/widget/src/constants/bridgeMap.ts (2 hunks)
- packages/widget/src/constants/bridgeable.ts (1 hunks)
🔇 Additional comments (9)
packages/synapse-interface/constants/tokens/bridgeable.ts (1)
46-46: LGTM: Import statement follows established pattern.The BNB logo import follows the same consistent pattern as other token logo imports.
packages/rest-api/src/constants/bridgeable.ts (1)
1434-1450: LGTM! The BNB token definition follows the established patterns.The implementation correctly defines BNB as a native token with appropriate properties matching other major tokens in the system.
packages/widget/src/constants/bridgeable.ts (1)
1454-1469: LGTM! The BNB token implementation looks correct and consistent.The implementation follows the established pattern for native tokens, using the appropriate special address for native BNB and correct token properties (18 decimals, priority rank 300).
packages/rest-api/src/constants/bridgeMap.ts (1)
572-578: Verify the BNB token configuration.The BNB token entry looks correct with:
- Proper canonical zero address for native token
- Correct decimals (18)
- RFQ bridge support via origin
However, the empty destination array suggests one-way bridging only. Please verify if this is the intended behavior.
✅ Verification successful
BNB token configuration is consistent across the codebase
The empty destination and swappable arrays are consistent across all bridgeMap.ts files in different packages (rest-api, synapse-constants, synapse-interface, and widget). This confirms that the one-way bridging configuration via RFQ.BNB is intentional and properly synchronized across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other files reference BNB bridging # Test: Search for BNB bridging references. Expect: Consistent one-way configuration. rg -l "RFQ.BNB"Length of output: 207
Script:
#!/bin/bash # Check the content of BNB configurations across different bridge map files for file in $(rg -l "RFQ.BNB"); do echo "=== $file ===" rg -A5 -B1 "'0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE'.*{" "$file" doneLength of output: 1732
Script:
#!/bin/bash # Check the content of BNB configurations across different bridge map files for file in $(rg -l "RFQ.BNB"); do echo "=== $file ===" rg -A5 "RFQ\.BNB" "$file" doneLength of output: 2116
packages/synapse-constants/src/constants/bridgeMap.ts (2)
593-599: Verify the WBNB token configuration.The WBNB token configuration looks correct with:
- Proper contract address for WBNB on BNB Chain
- Correct decimals (18)
- Origin set to 'RFQ.BNB'
However, similar to native BNB, having an empty destination array might limit bridging capabilities. Consider if WBNB should be bridgeable to any destinations.
Let's verify if WBNB should be bridgeable to any destinations:
✅ Verification successful
WBNB configuration is correctly set with empty destinations
Based on the verification results, the current WBNB configuration is correct as is. The search results show that:
- Both BNB and WBNB tokens are configured with
origin: ['RFQ.BNB']and empty destinations- This pattern is consistent across BNB-related tokens in the bridgeMap
- No other tokens in the bridgeMap reference BNB or WBNB as destinations
The empty destination array is intentional for RFQ-based tokens and doesn't limit functionality, as these tokens are handled through the RFQ system rather than traditional bridging paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if WBNB token is referenced as a destination in other chains # This helps verify if we need to add any destinations for WBNB rg -l "destination.*WBNB" --type tsLength of output: 37
Script:
#!/bin/bash # Let's check for any references to WBNB in the bridgeMap file to understand the bridging patterns rg -A 5 -B 5 "WBNB" packages/synapse-constants/src/constants/bridgeMap.ts # Also check for any BNB-related bridging patterns to compare with WBNB rg -A 5 -B 5 "BNB['\"].*destination" packages/synapse-constants/src/constants/bridgeMap.tsLength of output: 448
Script:
#!/bin/bash # Let's check for any BNB-related tokens and their bridging configurations rg "symbol.*BNB.*origin.*destination" -A 5 -B 2 packages/synapse-constants/src/constants/bridgeMap.ts # Also check for RFQ.BNB patterns to understand similar configurations rg "RFQ\.BNB" -A 5 -B 2 packages/synapse-constants/src/constants/bridgeMap.ts # Check for any bridge destinations that might be relevant for BNB/WBNB rg "destination.*\[.*BNB" packages/synapse-constants/src/constants/bridgeMap.tsLength of output: 651
572-578: Verify the BNB token configuration.The BNB token configuration looks correct with:
- Proper address for native BNB token
- Correct decimals (18)
- Origin set to 'RFQ.BNB'
However, having an empty destination array might limit bridging capabilities. Consider if BNB should be bridgeable to any destinations.
Let's verify if BNB should be bridgeable to any destinations:
✅ Verification successful
Empty destination array is correct for BNB token configuration
The empty destination array in the BNB token configuration is correct. From analyzing the codebase:
- Tokens with RFQ (Request for Quote) origins typically have empty destinations (e.g., WETH with 'RFQ.ETH' origin)
- BNB is configured with 'RFQ.BNB' origin, following the same pattern as other RFQ tokens
- This is consistent with other similar configurations in the bridgeMap, such as USDT and WETH with RFQ origins
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if BNB token is referenced as a destination in other chains # This helps verify if we need to add any destinations for BNB rg -l "destination.*BNB" --type tsLength of output: 37
Script:
#!/bin/bash # Let's try a broader search to understand BNB bridging patterns rg "BNB.*destination|destination.*BNB" -A 2 -B 2 --type ts # Also search for any BNB-related configurations rg "symbol.*BNB" -A 5 -B 2 --type tsLength of output: 9615
Script:
#!/bin/bash # Let's check if other tokens in the bridgeMap have destinations set # This will help us understand if empty destinations is the normal pattern rg "destination: \[.*\]" -B 2 packages/synapse-constants/src/constants/bridgeMap.tsLength of output: 19661
packages/synapse-interface/constants/bridgeMap.ts (1)
593-599: Verify WBNB configuration matches other wrapped native tokens.The WBNB token entry uses:
- Correct WBNB contract address on BNB Chain
- Same configuration as BNB (RFQ.BNB origin, empty destination/swappable)
Consider if WBNB should have additional swappable tokens defined to improve liquidity options.
#!/bin/bash # Description: Compare WBNB configuration with other wrapped native tokens # This helps validate if the configuration follows the same pattern echo "Searching for other wrapped native token configurations..." rg -U "symbol: 'W[A-Z]+'.+?origin: \['RFQ\." -A 5packages/widget/src/constants/bridgeMap.ts (2)
593-599: Verify the WBNB token configuration.The WBNB token configuration looks correct with:
- Standard WBNB contract address on BNB Chain (
0xbb4CdB9CBd36B01bD1cBaEBF2De08d9173bc095c)- Matching configuration with native BNB
#!/bin/bash # Description: Verify WBNB token address and configuration in other bridge-related files # Expect: Same configuration in other bridge map files and consistent with BNB # Search for WBNB token configurations rg --type ts "0xbb4CdB9CBd36B01bD1cBaEBF2De08d9173bc095c.*WBNB.*RFQ\.BNB"
572-578: Verify the BNB token configuration.The native BNB token configuration looks correct with:
- Standard address for native token (
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE)- Correct decimals (18)
- RFQ bridge support via
RFQ.BNB✅ Verification successful
BNB token configuration is correctly implemented across the codebase
The native BNB token configuration is consistent across multiple packages with:
- Standard native token address (
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE)- Correct symbol ("BNB") and decimals (18)
- RFQ bridge support via
RFQ.BNBprotocol- Configuration is synchronized in bridgeMap files across packages (widget, synapse-interface, rest-api)
- Proper integration with the BNB Chain configuration (chain ID 56)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify BNB token address and configuration in other bridge-related files # Expect: Same configuration in other bridge map files # Search for BNB token configurations rg --type ts "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE.*BNB.*RFQ\.BNB"Length of output: 75
Script:
#!/bin/bash # Let's try a broader search for BNB token configurations # Search for the native token address pattern rg --type ts "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE" # Search for BNB symbol usage in bridge configurations rg --type ts "symbol: ['\"](BNB)['\"]" # Search for RFQ.BNB pattern rg --type ts "RFQ\.BNB" # Look for any bridge-related configuration files fd "bridge.*\.(ts|js|json)$"Length of output: 20459
| export const BNB = new Token({ | ||
| priorityRank: 300, | ||
| visibilityRank: 300, | ||
| addresses: { | ||
| [CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE', | ||
| }, | ||
| decimals: { | ||
| [CHAINS.BNB.id]: 18, | ||
| }, | ||
| symbol: 'BNB', | ||
| name: 'BNB', | ||
| logo: bnbLogo, | ||
| swapableType: 'BNB', | ||
| color: 'yellow', | ||
| routeSymbol: 'BNB', | ||
| }) |
There was a problem hiding this comment.
Add missing isNative flag and standardize native token address.
The BNB token declaration should follow the same pattern as other native tokens (ETH, AVAX, KLAY, etc.).
Apply these changes to align with other native token declarations:
export const BNB = new Token({
priorityRank: 300,
visibilityRank: 300,
addresses: {
- [CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE',
+ [CHAINS.BNB.id]: zeroAddress,
},
decimals: {
[CHAINS.BNB.id]: 18,
},
symbol: 'BNB',
name: 'BNB',
logo: bnbLogo,
+ isNative: true,
swapableType: 'BNB',
color: 'yellow',
routeSymbol: 'BNB',
})Committable suggestion was skipped due to low confidence.
| export const BNB = new Token({ | ||
| priorityRank: 300, | ||
| addresses: { | ||
| [CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE', | ||
| }, | ||
| decimals: { | ||
| [CHAINS.BNB.id]: 18, | ||
| }, | ||
| symbol: 'BNB', | ||
| name: 'BNB', | ||
| swapableType: 'BNB', | ||
| color: 'yellow', | ||
| routeSymbol: 'BNB', | ||
| logo: 'https://45a97b3d.sanguine-fe.pages.dev/_next/static/media/bnb.5948fe5e.svg', | ||
| }) |
There was a problem hiding this comment.
Add missing properties to BNB token definition
The BNB token definition is missing several standard properties that are commonly used across other token definitions in this file:
isNativeflag should be set totrueas BNB is the native token on BNB chain- Missing standard properties that should be added:
coingeckoIdvisibilityRankswapableOn
Apply this diff to add the missing properties:
export const BNB = new Token({
priorityRank: 300,
addresses: {
[CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE',
},
decimals: {
[CHAINS.BNB.id]: 18,
},
symbol: 'BNB',
name: 'BNB',
swapableType: 'BNB',
color: 'yellow',
routeSymbol: 'BNB',
logo: 'https://45a97b3d.sanguine-fe.pages.dev/_next/static/media/bnb.5948fe5e.svg',
+ isNative: true,
+ coingeckoId: 'binancecoin',
+ visibilityRank: 90,
+ swapableOn: [CHAINS.BNB.id],
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const BNB = new Token({ | |
| priorityRank: 300, | |
| addresses: { | |
| [CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE', | |
| }, | |
| decimals: { | |
| [CHAINS.BNB.id]: 18, | |
| }, | |
| symbol: 'BNB', | |
| name: 'BNB', | |
| swapableType: 'BNB', | |
| color: 'yellow', | |
| routeSymbol: 'BNB', | |
| logo: 'https://45a97b3d.sanguine-fe.pages.dev/_next/static/media/bnb.5948fe5e.svg', | |
| }) | |
| export const BNB = new Token({ | |
| priorityRank: 300, | |
| addresses: { | |
| [CHAINS.BNB.id]: '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE', | |
| }, | |
| decimals: { | |
| [CHAINS.BNB.id]: 18, | |
| }, | |
| symbol: 'BNB', | |
| name: 'BNB', | |
| swapableType: 'BNB', | |
| color: 'yellow', | |
| routeSymbol: 'BNB', | |
| logo: 'https://45a97b3d.sanguine-fe.pages.dev/_next/static/media/bnb.5948fe5e.svg', | |
| isNative: true, | |
| coingeckoId: 'binancecoin', | |
| visibilityRank: 90, | |
| swapableOn: [CHAINS.BNB.id], | |
| }) |
| '0xbb4CdB9CBd36B01bD1cBaEBF2De08d9173bc095c': { | ||
| decimals: 18, | ||
| symbol: 'WBNB', | ||
| origin: ['RFQ.BNB'], | ||
| destination: [], | ||
| swappable: [], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing WBNB integration.
While the WBNB configuration mirrors BNB's settings, consider these potential improvements:
- Add swappable tokens to enable DEX integrations
- Consider adding destination chains if two-way bridging should be supported
Example enhancement:
'0xbb4CdB9CBd36B01bD1cBaEBF2De08d9173bc095c': {
decimals: 18,
symbol: 'WBNB',
origin: ['RFQ.BNB'],
destination: [],
- swappable: [],
+ swappable: [
+ '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE' // Native BNB
+ ],
},Committable suggestion was skipped due to low confidence.
| '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE': { | ||
| decimals: 18, | ||
| symbol: 'BNB', | ||
| origin: ['RFQ.BNB'], | ||
| destination: [], | ||
| swappable: [], | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Empty destination array for BNB is inconsistent with other RFQ tokens
Looking at other RFQ token configurations in the bridgeMap:
- ETH has both origin and destination set to ['RFQ.ETH']
- USDC has both origin and destination set to ['RFQ.USDC']
- WLD has both origin and destination set to ['RFQ.WLD']
The BNB configuration should follow the same pattern and include 'RFQ.BNB' in both origin and destination arrays to enable bi-directional bridging.
packages/synapse-interface/constants/bridgeMap.ts: Update BNB and WBNB entries to includedestination: ['RFQ.BNB']
🔗 Analysis chain
Verify the BNB bridging configuration.
The BNB token entry is added with:
- Correct zero address for native token
- Correct decimals (18)
- RFQ.BNB as origin
- Empty destination and swappable arrays
Consider if the empty destination array is intentional, as it means BNB cannot be the destination token in any bridge operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if other RFQ tokens have similar empty destination arrays
# This helps validate if the configuration aligns with other RFQ implementations
echo "Searching for other RFQ token configurations..."
rg -U "origin: \['RFQ\.[^']+'\][^{]*destination: \[\]"
Length of output: 281
Script:
#!/bin/bash
# Let's try a different approach to search for RFQ configurations
echo "Searching for RFQ token configurations..."
rg -U "origin: \[\s*'RFQ\." -A 3 -B 3 packages/synapse-interface/constants/bridgeMap.ts
echo -e "\nSearching for other native token configurations..."
rg -U "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE" -A 5 -B 2 packages/synapse-interface/constants/bridgeMap.ts
Length of output: 12311
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rest-api/src/tests/tokenListRoute.test.ts (1)
Line range hint
17-24: Add BNB token verification to the test case.While the test verifies the total token count and existing tokens (ETH, SYN), it should also validate the newly added BNB token address to ensure correct mapping.
Add BNB verification like this:
expect(keys.length).toBe(65) expect(response.body['ETH']['addresses']['1']).toBe( '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE' ) expect(response.body['SYN']['addresses']['1']).toBe( '0x0f2d719407fdbeff09d87557abb7232601fd9f29' ) + expect(response.body['BNB']['addresses']['56']).toBe( + '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE' + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/rest-api/src/tests/indexRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/tokenListRoute.test.ts (1 hunks)
- packages/synapse-interface/constants/tokens/bridgeable.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/constants/tokens/bridgeable.ts
🧰 Additional context used
📓 Learnings (1)
packages/rest-api/src/tests/indexRoute.test.ts (4)
Learnt from: abtestingalpha PR: synapsecns/sanguine#3126 File: packages/rest-api/src/tests/indexRoute.test.ts:21-31 Timestamp: 2024-10-08T15:41:49.333Z Learning: In this project, tests intentionally use hardcoded values for counts to capture changes in the number of available chains and tokens.Learnt from: abtestingalpha PR: synapsecns/sanguine#3126 File: packages/rest-api/src/tests/indexRoute.test.ts:21-31 Timestamp: 2024-09-16T17:19:34.740Z Learning: In this project, tests intentionally use hardcoded values for counts to capture changes in the number of available chains and tokens.Learnt from: abtestingalpha PR: synapsecns/sanguine#3126 File: packages/rest-api/src/tests/indexRoute.test.ts:21-31 Timestamp: 2024-10-08T15:41:49.333Z Learning: In this project, tests intentionally use hardcoded values for counts to capture changes in the number of available chains and tokens.Learnt from: abtestingalpha PR: synapsecns/sanguine#3126 File: packages/rest-api/src/tests/indexRoute.test.ts:21-31 Timestamp: 2024-10-08T15:41:48.274Z Learning: In this project, tests intentionally use hardcoded values for counts to capture changes in the number of available chains and tokens.
🔇 Additional comments (2)
packages/rest-api/src/tests/tokenListRoute.test.ts (1)
17-17: LGTM! The token count update aligns with BNB addition.The increment from 64 to 65 tokens correctly reflects the addition of the BNB token to the bridge map.
packages/rest-api/src/tests/indexRoute.test.ts (1)
30-30: LGTM! The updated token count correctly reflects the addition of BNB.The increment from 64 to 65 tokens aligns with the PR's objective of adding BNB token support. Note: As per project convention, we intentionally use hardcoded values in these tests to catch unexpected changes in the number of available tokens.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3338 +/- ##
====================================================
+ Coverage 33.25951% 90.44834% +57.18883%
====================================================
Files 543 54 -489
Lines 34769 1026 -33743
Branches 82 82
====================================================
- Hits 11564 928 -10636
+ Misses 22182 95 -22087
+ Partials 1023 3 -1020
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Last item here is to get the svg static image for the token and include that in the rest API and constants package. |
This reverts commit b80adfc.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/synapse-interface/components/Activity/Transaction/TransactionOptions.tsx (1)
Line range hint
142-146: Improve type safety of TransactionPopoverContainer props.While the component architecture is well-structured, the
childrenprop is typed asany, which reduces type safety. Consider using a more specific type definition.export function TransactionPopoverContainer({ children, className, }: { - children: any + children: React.ReactNode className?: string })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/synapse-interface/assets/icons/bnb.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
- packages/synapse-interface/components/Activity/Transaction/TransactionOptions.tsx (1 hunks)
- packages/synapse-interface/components/Pools/PoolActionOptions.tsx (1 hunks)
- packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsx (2 hunks)
🔇 Additional comments (1)
packages/synapse-interface/components/Pools/PoolActionOptions.tsx (1)
123-123: 🛠️ Refactor suggestionConsider a more type-safe alternative to
as any.While this change resolves type compatibility issues with @headlessui/react, using
as anybypasses TypeScript's type checking system, which could mask potential type-related bugs.Consider these alternatives:
- Update @headlessui/react to the latest version which might have better TypeScript support
- Create a proper type definition:
import { TransitionRootProps } from '@headlessui/react' type CustomTransitionProps = Omit<TransitionRootProps, 'as'> & { as: typeof Fragment }Let's verify if this is a broader pattern:
| @@ -145,7 +145,7 @@ export function TransactionPopoverContainer({ | |||
| }) { | |||
| return ( | |||
| <Transition | |||
| as={Fragment} | |||
| as={Fragment as any} | |||
There was a problem hiding this comment.
Avoid using as any type assertion.
The type assertion Fragment as any bypasses TypeScript's type checking system, which could mask potential type-related issues. This is generally considered an anti-pattern in TypeScript development.
Consider one of these type-safe alternatives:
- as={Fragment as any}
+ as={Fragment}Or if you're facing type compatibility issues with @headlessui/react, properly type it using:
import { TransitionRootProps } from '@headlessui/react'
type AsProps = TransitionRootProps['as']
// Then use it as
as={Fragment as AsProps}| @@ -118,7 +118,7 @@ export function LandingNav() { | |||
| </div> | |||
|
|
|||
| <Transition | |||
| as={Fragment} | |||
| as={Fragment as any} | |||
There was a problem hiding this comment.
Avoid using as any type assertions with @headlessui/react components.
The type assertions Fragment as any in both Transition components bypass TypeScript's type checking, which could hide potential runtime errors. This is generally considered an anti-pattern.
Consider these alternatives:
- Use proper types from @headlessui/react:
import { Transition } from '@headlessui/react'
import type { Fragment } from 'react'
// Use proper type
as={Fragment as React.ComponentType}- If the type issue persists, check for version mismatches:
npm info @headlessui/react version
npm info @types/react version- Update dependencies if needed:
npm update @headlessui/react @types/reactAlso applies to: 164-164
Defi-Moses
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- packages/rest-api/src/constants/bridgeMap.ts (1 hunks)
- packages/rest-api/src/constants/bridgeable.ts (2 hunks)
- packages/synapse-constants/src/constants/bridgeMap.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/bridgeable.ts (1 hunks)
- packages/synapse-interface/constants/bridgeMap.ts (1 hunks)
- packages/synapse-interface/constants/tokens/bridgeable.ts (1 hunks)
- packages/widget/src/constants/bridgeMap.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rest-api/src/constants/bridgeMap.ts
- packages/synapse-interface/constants/bridgeMap.ts
- packages/synapse-interface/constants/tokens/bridgeable.ts
🔇 Additional comments (2)
packages/rest-api/src/constants/bridgeable.ts (1)
811-811: Implementation looks good!The addition of ETH token support for the BNB chain is properly implemented with:
- Correct contract address for the ETH token on BNB chain
- Standard decimals value (18) consistent with other chains
Let's verify the ETH token contract address on BNB chain:
Also applies to: 825-825
✅ Verification successful
ETH token address on BNB chain is correctly configured
The implementation is consistent across the codebase:
- The ETH token contract address
0x2170Ed0880ac9A755fd29B2688956BD959F933F8is properly referenced in all relevant configuration files- The token is consistently configured with 18 decimals
- The symbol is correctly set as 'ETH' in bridge mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the ETH token contract address on BNB chain # Expected: Should find references to this contract address in bridge configuration rg -l "0x2170Ed0880ac9A755fd29B2688956BD959F933F8"Length of output: 408
Script:
#!/bin/bash # Let's verify the consistency of ETH token address across different files rg "0x2170Ed0880ac9A755fd29B2688956BD959F933F8" -A 2 -B 2Length of output: 2861
packages/widget/src/constants/bridgeMap.ts (1)
525-531: Verify the token configuration.The ETH token configuration has the following potential issues:
- Empty destination array might prevent bridging to other chains
- No swappable tokens might limit functionality
- Only 'RFQ.ETH' in origin array might restrict bridging options
Please verify if these configurations are intentional.
Run the following script to check ETH token configurations across other chains:
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough information about ETH token configurations across the codebase:
ETH token configuration is consistent with other implementations
The empty destination array and swappable array for this ETH token (0x2170Ed0880ac9A755fd29B2688956BD959F933F8) is intentional as it follows a pattern seen in other similar configurations where:
- Tokens with only 'RFQ.ETH' in origin typically have either empty destination arrays or ['RFQ.ETH'] as destination
- Not all ETH tokens require swappable configurations, as evidenced by multiple other ETH token entries with empty swappable arrays
- The configuration aligns with other ETH tokens in the bridgeMap that have similar minimal configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check ETH token configurations across chains for consistency # Test: Search for ETH token entries with RFQ.ETH origin echo "ETH tokens with RFQ.ETH origin:" rg -A 5 "'RFQ.ETH'" # Test: Search for ETH symbol entries echo "Tokens with ETH symbol:" rg -A 5 "symbol: 'ETH'"Length of output: 80400
| "viem": "^2.21.6", | ||
| "@codecov/rollup-plugin": "^1.2.0" |
There was a problem hiding this comment.
Move @codecov/rollup-plugin to devDependencies
The @codecov/rollup-plugin is a development tool for code coverage reporting and should be in devDependencies rather than dependencies. This change will prevent the package from being installed in production environments where it's not needed.
Apply this diff to fix the dependency location:
"dependencies": {
"viem": "^2.21.6",
- "@codecov/rollup-plugin": "^1.2.0"
},
"devDependencies": {
+ "@codecov/rollup-plugin": "^1.2.0"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "viem": "^2.21.6", | |
| "@codecov/rollup-plugin": "^1.2.0" | |
| "viem": "^2.21.6" |
💡 Codebase verification
Code coverage plugin addition appears unrelated to current changes
The addition of @codecov/rollup-plugin to synapse-constants package appears unnecessary as:
- There are no test files in the synapse-constants package
- No changes were detected in the rollup configuration to utilize this plugin
- The package's purpose is to provide constants, which typically don't require code coverage
Consider:
- Removing the
@codecov/rollup-plugindependency as it's not being used - If code coverage is needed, create a separate PR with proper test setup and configuration
🔗 Analysis chain
Verify the relevance of code coverage changes
The addition of the code coverage plugin appears unrelated to the PR's objective of adding ETH on BNB Chain. Could you clarify why these changes are included in this PR? Consider moving them to a separate PR focused on testing infrastructure improvements.
Let me gather more specific information about code coverage related changes.
Let me check the specific rollup configuration changes in the synapse-constants package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any test-related changes in this PR that would justify adding code coverage
rg -l "test|spec|coverage" --type ts --type js
Length of output: 6964
Script:
#!/bin/bash
# Check for any changes to test configuration files or coverage settings
git diff HEAD~1 -G"coverage|jest|test" -- "*.json" "*.js" "*.ts"
# Check if codecov plugin is used in any rollup configs
rg -l "codecov|rollup" --type js --type ts
Length of output: 363
Script:
#!/bin/bash
# Check the rollup config changes in synapse-constants
git diff HEAD~1 packages/synapse-constants/rollup.config.js
# Also check if there are any test files in synapse-constants
fd -t f "test|spec" packages/synapse-constants/
Length of output: 111
This PR adds the BNB token to bridgeMap and bridgeable tokens lists. A relayer now supports BNB (on BNB chain) <> ETH (on any chain) routes.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
de6b5da: synapse-interface preview link
913d0c9: synapse-interface preview link