-
Notifications
You must be signed in to change notification settings - Fork 22
add: xdc support #603
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
add: xdc support #603
Conversation
Deploying goodprotocolui with
|
| Latest commit: |
66ab226
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07af597a.goodprotocolui.pages.dev |
| Branch Preview URL: | https://xdc-upgrade.goodprotocolui.pages.dev |
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 by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Unsafe Network Name Processing ▹ view | ||
| Static XDC Gas Price ▹ view | ✅ Fix detected | |
| Exposed API Key in Frontend Code ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/utils/env.ts | ✅ |
| src/constants/multicall/index.ts | ✅ |
| src/components/BlockNativeOnboard/index.tsx | ✅ |
| src/constants/networks.ts | ✅ |
| src/pages/gd/Claim/Claim.tsx | ✅ |
| src/components/CurrencyLogo/index.tsx | ✅ |
| src/pages/gd/Claim/ClaimBalance.tsx | ✅ |
| src/hooks/useWeb3.tsx | ✅ |
| src/connectors/index.ts | ✅ |
| src/components/NetworkModal/index.tsx | ✅ |
| src/components/SideBar.tsx | ✅ |
| src/constants/index.ts | ✅ |
| src/utils/index.ts | ✅ |
| src/components/Faq/copies.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
src/hooks/useWeb3.tsx
Outdated
| if (chainId === (50 as ChainId) && method === 'eth_sendTransaction') { | ||
| params[0].gasPrice = BigNumber.from(12.5e9).toHexString() | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| MAINNET: process.env.REACT_APP_GOODID_MAINNET_EXPLORER ?? '', | ||
| XDC: | ||
| process.env.REACT_APP_GOODID_XDC_EXPLORER ?? | ||
| `https://api.etherscan.io/v2/api?chainid=50&apikey=${process.env.REACT_APP_ETHERSCAN_KEY}&`, |
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.
Exposed API Key in Frontend Code 
Tell me more
What is the issue?
API key is being exposed in client-side code where it could be extracted by end users.
Why this matters
Client-side exposure of API keys allows unauthorized usage, potential abuse, and billing implications. API requests should be proxied through a backend service.
Suggested change ∙ Feature Preview
Move API calls to a backend service and create an authenticated endpoint that proxies requests to Etherscan:
// Frontend
const goodIdExplorerUrls = {
CELO: process.env.REACT_APP_GOODID_CELO_EXPLORER ?? 'your-backend-api/explorer/celo',
XDC: process.env.REACT_APP_GOODID_XDC_EXPLORER ?? 'your-backend-api/explorer/xdc'
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if (network.includes('-')) { | ||
| const baseNetwork = network.split('-')[0] | ||
| return baseNetwork | ||
| } |
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.
Unsafe Network Name Processing 
Tell me more
What is the issue?
The code assumes all network names with a hyphen follow the same format and only takes the first part, which could lead to incorrect environment determination for networks with different naming patterns.
Why this matters
If new networks are added with different naming patterns containing hyphens (e.g., 'test-xdc-network'), the function will return 'test' instead of the intended environment, potentially causing the application to run in the wrong environment.
Suggested change ∙ Feature Preview
Create a whitelist of known network prefixes or implement a more robust network name parsing logic. Example:
if (network.includes('-')) {
// Whitelist approach
const knownNetworks = ['development', 'staging', 'production'];
const prefix = network.split('-')[0];
return knownNetworks.includes(prefix) ? prefix : defaultValue;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
- fix switch-chain on claim-alt - on xdc, always go to old claim flow (no goodid)
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/utils/index.ts:251` </location>
<code_context>
- },
- fuse: (chainName = '', data: string, type: 'transaction' | 'token' | 'address' | 'block') => {
- const prefix = 'https://explorer.fuse.io/'
+ celo: (chainName = '', data: string, type: 'transaction' | 'token' | 'address' | 'block') => {
+ const prefix = 'https://celoscan.io/'
switch (type) {
</code_context>
<issue_to_address>
The builder for XDC uses a trailing slash in the prefix; check for double slashes in generated URLs.
Consider removing the trailing slash from the prefix to prevent double slashes in the generated URLs.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const prefix = 'https://celoscan.io/'
=======
const prefix = 'https://celoscan.io'
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/pages/gd/Claim/index.tsx:27` </location>
<code_context>
if (payload === undefined || isGoodIdEnabled === undefined) return <Spinner variant="page-loader" size="lg" />
- return isGoodIdEnabled && !isMiniPay ? <NewClaim /> : <OldClaim />
+ return isGoodIdEnabled && !isMiniPay && chainId !== 50 ? <NewClaim /> : <OldClaim />
}
</code_context>
<issue_to_address>
The claim page disables NewClaim for chainId 50 (XDC); clarify rationale.
If XDC is intentionally excluded, please add a comment or TODO explaining the reason for future reference.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fuse: (chainName = '', data: string, type: 'transaction' | 'token' | 'address' | 'block') => { | ||
| const prefix = 'https://explorer.fuse.io/' | ||
| celo: (chainName = '', data: string, type: 'transaction' | 'token' | 'address' | 'block') => { | ||
| const prefix = 'https://celoscan.io/' |
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.
suggestion: The builder for XDC uses a trailing slash in the prefix; check for double slashes in generated URLs.
Consider removing the trailing slash from the prefix to prevent double slashes in the generated URLs.
| const prefix = 'https://celoscan.io/' | |
| const prefix = 'https://celoscan.io' |
| if (payload === undefined || isGoodIdEnabled === undefined) return <Spinner variant="page-loader" size="lg" /> | ||
|
|
||
| return isGoodIdEnabled && !isMiniPay ? <NewClaim /> : <OldClaim /> | ||
| return isGoodIdEnabled && !isMiniPay && chainId !== 50 ? <NewClaim /> : <OldClaim /> |
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.
question: The claim page disables NewClaim for chainId 50 (XDC); clarify rationale.
If XDC is intentionally excluded, please add a comment or TODO explaining the reason for future reference.
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 by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Static XDC Gas Price ▹ view | ✅ Fix detected | |
| Incorrect XDC Fallback RPC URL ▹ view | ||
| Incorrect Environment Detection for Hyphenated Network Names ▹ view | ||
| Inverted Claim Availability Logic ▹ view | ||
| Hardcoded Chain-specific Gas Prices ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| src/utils/env.ts | ✅ |
| src/pages/gd/Claim/index.tsx | ✅ |
| src/components/Web3SupportedNetworks/index.tsx | ✅ |
| src/constants/multicall/index.ts | ✅ |
| src/components/BlockNativeOnboard/index.tsx | ✅ |
| src/constants/networks.ts | ✅ |
| src/pages/gd/Claim/Claim.tsx | ✅ |
| src/components/CurrencyLogo/index.tsx | ✅ |
| src/components/Savings/SavingsCard/index.tsx | ✅ |
| src/pages/gd/Stake/Savings/index.tsx | ✅ |
| src/pages/gd/Claim/ClaimBalance.tsx | ✅ |
| src/hooks/useWeb3.tsx | ✅ |
| src/connectors/index.ts | ✅ |
| src/components/NetworkModal/index.tsx | ✅ |
| src/components/SideBar.tsx | ✅ |
| src/constants/index.ts | ✅ |
| src/utils/index.ts | ✅ |
| src/components/Faq/copies.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
src/hooks/useWeb3.tsx
Outdated
| if (chainId === (50 as ChainId) && method === 'eth_sendTransaction') { | ||
| params[0].gasPrice = BigNumber.from(12.5e9).toHexString() | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (!isMiniPay && chainId === (42220 as ChainId) && method === 'eth_sendTransaction') { | ||
| params[0].gasPrice = BigNumber.from(25.001e9).toHexString() | ||
| } | ||
|
|
||
| if (chainId === (122 as ChainId) && method === 'eth_sendTransaction') { | ||
| params[0].gasPrice = BigNumber.from(11e9).toHexString() | ||
| } | ||
|
|
||
| if (chainId === (50 as ChainId) && method === 'eth_sendTransaction') { | ||
| params[0].gasPrice = BigNumber.from(12.5e9).toHexString() | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (!claimedCelo?.isZero() && Number(chainId) !== SupportedChains.CELO) { | ||
| return setClaimNext(SupportedChains.CELO) | ||
| } | ||
| if (!claimedXdc?.isZero() && Number(chainId) !== SupportedChains.XDC) { | ||
| return setClaimNext(SupportedChains.XDC) | ||
| } | ||
| if (!claimedFuse?.isZero() && Number(chainId) !== SupportedChains.FUSE) { | ||
| return setClaimNext(SupportedChains.FUSE) | ||
| } else setClaimNext(undefined) |
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.
Inverted Claim Availability Logic 
Tell me more
What is the issue?
The logic for determining the next chain to claim on is incorrect. It sets the next chain when a claim is NOT zero (meaning already claimed) instead of when it IS zero (available to claim).
Why this matters
Users will be directed to chains where they've already claimed instead of chains where claims are available, preventing them from accessing their available claims.
Suggested change ∙ Feature Preview
Invert the logic to check for zero claims:
if (claimedCelo?.isZero() && Number(chainId) !== SupportedChains.CELO) {
return setClaimNext(SupportedChains.CELO)
}
if (claimedXdc?.isZero() && Number(chainId) !== SupportedChains.XDC) {
return setClaimNext(SupportedChains.XDC)
}
if (claimedFuse?.isZero() && Number(chainId) !== SupportedChains.FUSE) {
return setClaimNext(SupportedChains.FUSE)
} else setClaimNext(undefined)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
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.
Wrong, isZero here means there is no available claim.
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.
I apologize for the confusion. You are correct - isZero() indicates no available claim, so the current logic is appropriate. Please disregard my previous suggestion.
Description
Added support for dev env xdc
requires matching branch from goodweb3-mono
To test
Description by Korbit AI
What change is being made?
Add support for the XDC network across the codebase, including dependencies, logo assets, supported chain configuration, network modals, and explorer URL handling.
Why are these changes being made?
These changes enable the application to support the XDC network, which is crucial for broadening the scope of decentralized technology access across different blockchain networks. The updates allow the system to recognize XDC as a native currency, handle network-specific connections, display logos, and manage transactions through XDC's block explorer. Limitations include network-specific features not being optimized yet, requiring future refinement of network-specific functionalities.