fix(xchain-suite): address Chainflip CR feedback#1616
Merged
Thorian1te merged 2 commits intomasterfrom Feb 23, 2026
Merged
Conversation
- Normalize 'SENT' state to 'COMPLETED' to fix broken stage visualization - Move requestDepositAddressV2 from estimateSwap to doSwap to avoid premature channel opening - Fix copy button copying txHash instead of depositChannelId for Chainflip - Use getChainflipSdk singleton in PoolsPage and SwapTrackingModal - Replace redundant dynamic import with static CryptoAmount/baseAmount imports - Use ChainflipBoost type instead of 'as any' casts - Add try/catch around clipboard API calls - Hide THORChain code examples when Chainflip quote is selected - Align SOL Stagenet RPC to mainnet (matching explorer config) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughCentralizes Chainflip SDK access via getChainflipSdk(), moves Chainflip deposit-address/channel handling from compile-time types to runtime (requestDepositAddressV2), removes Chainflip-specific fields from public Swap types, and adds async try/catch clipboard handling in relevant components. Several UI files update quote and swap execution flows accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/xchain-suite/src/lib/swap/SwapService.ts (1)
307-330: Extract the duplicated Chainflip quote-fetch block into a private helper.Lines 307–330 are nearly identical to the corresponding block inside
estimateSwap(chain/asset name lookup,amountStr,affiliateBrokers,getQuoteV2call,bestQuoteselection). A private helper likefetchBestChainflipQuote(params)would eliminate the duplication and keep both paths in sync.Note: lines 307–308 also silently produce
undefinedforsrcChainName/destChainNameifdoSwapis called directly withprotocol: 'Chainflip'and an unsupported chain. Adding anisChainflipCompatibleguard (matching the one at line 211 inestimateSwap) before line 307 would surface a clearer error message.♻️ Sketch of the extracted helper + guard
+ private async fetchBestChainflipQuote(params: SwapParams) { + const srcChain = params.fromAsset.chain + const destChain = params.destinationAsset.chain + if (!isChainflipCompatible(srcChain) || !isChainflipCompatible(destChain)) { + throw new Error(`Chainflip does not support chain pair: ${srcChain} → ${destChain}`) + } + const sdk = await getChainflipSdk() + const srcChainName = CHAINFLIP_CHAINS[srcChain] + const destChainName = CHAINFLIP_CHAINS[destChain] + const srcAsset = params.fromAsset.ticker + const destAsset = params.destinationAsset.ticker + const amountStr = params.amount.baseAmount.amount().toFixed(0) + const affiliateAddress = import.meta.env.VITE_ASGARDEX_AFFILIATE_BROKERS_ADDRESS + const affiliateBrokers = affiliateAddress + ? [{ account: affiliateAddress as `cF${string}` | `0x${string}`, commissionBps: 0 }] + : undefined + const quoteResponse = await sdk.getQuoteV2({ + srcChain: srcChainName, srcAsset, destChain: destChainName, destAsset, amount: amountStr, + ...(affiliateBrokers && { affiliateBrokers }), + }) + const cfQuotes = quoteResponse.quotes || [] + const bestQuote = cfQuotes.find((q: any) => q.type === 'REGULAR') || cfQuotes[0] + return { sdk, bestQuote, affiliateBrokers } + }Then
doSwapandestimateSwapboth callthis.fetchBestChainflipQuote(params).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/lib/swap/SwapService.ts` around lines 307 - 330, Extract the duplicated Chainflip quote-fetching logic into a private helper method (e.g., fetchBestChainflipQuote(params)) and update both doSwap and estimateSwap to call it; the helper should compute srcChainName/destChainName from CHAINFLIP_CHAINS, build amountStr and affiliateBrokers, call sdk.getQuoteV2, select the bestQuote (type === 'REGULAR' fallback to first), and throw a clear error if no quote is returned. Before calling the helper from doSwap, add the same isChainflipCompatible(params) guard used in estimateSwap to validate supported chains and throw a descriptive error if incompatible, so srcChainName/destChainName won't be undefined. Ensure the helper is private to the class and preserves current behavior and error messages where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/xchain-suite/src/components/swap/SwapTrackingModal.tsx`:
- Around line 236-244: The copy button's static tooltip ("Copy hash") is stale
because handleCopy now copies depositChannelId for Chainflip swaps; update the
copy button's title/tooltip prop to reflect the actual value being copied by
making it dynamic (e.g., use protocol === 'Chainflip' && depositChannelId ?
'Copy deposit channel id' : 'Copy tx hash' or include the copied string) so the
tooltip matches the text produced by handleCopy; locate the button component
that references the title="Copy hash" and replace it with a dynamic expression
tied to protocol, depositChannelId and txHash.
In `@tools/xchain-suite/src/pages/SwapPage.tsx`:
- Line 584: The JSX conditional expression in SwapPage.tsx is missing the
required spaces inside the curly braces which triggers Prettier/ESLint's "Insert
··" rule; update the JSX expression that uses fromAsset, toAsset, amount and
selectedQuote?.protocol (the line starting with the conditional render) to
include a space after the opening "{" and before the closing "}" (i.e., change
"{fromAsset && toAsset && amount && selectedQuote?.protocol !== 'Chainflip' &&
(" to use "{ fromAsset && toAsset && amount && selectedQuote?.protocol !==
'Chainflip' && (" and close with " ) }" style), and scan for any similar JSX
curly-brace expressions in SwapPage.tsx to apply the same spacing convention.
---
Nitpick comments:
In `@tools/xchain-suite/src/lib/swap/SwapService.ts`:
- Around line 307-330: Extract the duplicated Chainflip quote-fetching logic
into a private helper method (e.g., fetchBestChainflipQuote(params)) and update
both doSwap and estimateSwap to call it; the helper should compute
srcChainName/destChainName from CHAINFLIP_CHAINS, build amountStr and
affiliateBrokers, call sdk.getQuoteV2, select the bestQuote (type === 'REGULAR'
fallback to first), and throw a clear error if no quote is returned. Before
calling the helper from doSwap, add the same isChainflipCompatible(params) guard
used in estimateSwap to validate supported chains and throw a descriptive error
if incompatible, so srcChainName/destChainName won't be undefined. Ensure the
helper is private to the class and preserves current behavior and error messages
where appropriate.
- Make copy button tooltip dynamic (deposit channel ID vs tx hash) - Extract fetchBestChainflipQuote helper to deduplicate quote logic - Add isChainflipCompatible guard in doSwap path - Run prettier on SwapPage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
underthesun49
approved these changes
Feb 23, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tools/xchain-suite/src/pages/SwapPage.tsx (2)
86-86: Replaceanywith a proper balance type.ESLint flags
@typescript-eslint/no-explicit-anyhere. The balance entries have a known shape ({ asset: Asset, amount: BaseAmount }).Proposed fix
- (b: any) => b.asset.chain === asset.chain && b.asset.symbol === asset.symbol, + (b: { asset: Asset; amount: import('@xchainjs/xchain-util').BaseAmount }) => + b.asset.chain === asset.chain && b.asset.symbol === asset.symbol,Alternatively, import
Balancefrom@xchainjs/xchain-clientif available and use that type directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/pages/SwapPage.tsx` at line 86, The filter callback currently types its parameter as any; replace that with the correct balance type (e.g., Balance from `@xchainjs/xchain-client`) or an inline type matching the shape { asset: Asset; amount: BaseAmount } so ESLint no-explicit-any is satisfied; update the predicate (b: Balance) => b.asset.chain === asset.chain && b.asset.symbol === asset.symbol (or use the inline type) and add the appropriate import for Balance if using the exported type.
387-393: Redundant USD value computation inside the input overlay.
inputUsdValue(line 56) already holds the result ofprices.calculateValue(...). The inner call on line 389 re-computes the same value just to null-check it, but the outer guard on line 387 (inputUsdValue !== null && inputUsdValue > 0) already ensures a valid value. Simplify to just renderformatUsdValue(inputUsdValue).Proposed fix
{inputUsdValue !== null && inputUsdValue > 0 && ( <div className="absolute right-3 top-1/2 -translate-y-1/2 text-sm text-gray-400 dark:text-gray-500"> - {prices.calculateValue(parseFloat(amount) || 0, fromAssetObj!) !== null && ( - <span>~{formatUsdValue(inputUsdValue)}</span> - )} + <span>~{formatUsdValue(inputUsdValue)}</span> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/pages/SwapPage.tsx` around lines 387 - 393, The inner redundant call to prices.calculateValue(...) inside the input overlay should be removed: use the already-computed inputUsdValue (from earlier in SwapPage) as the single source of truth and render formatUsdValue(inputUsdValue) directly; update the JSX in SwapPage (the block that currently checks inputUsdValue !== null && inputUsdValue > 0 and then re-calls prices.calculateValue) to drop the nested calculateValue check and simply output ~{formatUsdValue(inputUsdValue)} when inputUsdValue is valid.tools/xchain-suite/src/lib/swap/SwapService.ts (3)
8-8: Mixed type/value imports from@xchainjs/xchain-util.Aliasing
CryptoAmountasCryptoAmountClassandbaseAmountasbaseAmountFnworks but can be confusing — the typeCryptoAmountand the classCryptoAmountClassrefer to the same thing in different roles. This is a TypeScript idiom for importing both the type and value under different names.Consider a brief inline comment for clarity, e.g.:
// CryptoAmountClass = runtime class; CryptoAmount = type-only alias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/lib/swap/SwapService.ts` at line 8, The import mixes type-only and runtime values from '@xchainjs/xchain-util' which can be confusing: CryptoAmount is imported as a type and also aliased as CryptoAmountClass for the runtime value, and baseAmount is aliased to baseAmountFn; add a short inline comment next to the import (or directly above it) clarifying that CryptoAmount is the type-only alias while CryptoAmountClass is the runtime class, and that baseAmountFn is the runtime function alias for baseAmount so future readers understand the dual type/value usage (refer to CryptoAmount, CryptoAmountClass, baseAmountFn in SwapService.ts).
303-332: Re-fetching the quote indoSwapis correct but may surprise the user.Calling
fetchBestChainflipQuoteagain at execution time ensures a fresh quote and avoids prematurely opening deposit channels (the core PR fix). However, this means the actual swap parameters (egress amount, slippage) can differ from what the user confirmed in the modal.The
fillOrKillParamson lines 311-315 provide protection viaslippageTolerancePercentandrefundAddress, so funds aren't lost if the market moves. This trade-off is reasonable.Consider logging or surfacing a warning if the fresh quote's
egressAmountdiffers significantly from the original estimate shown to the user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/lib/swap/SwapService.ts` around lines 303 - 332, The code re-fetches a Chainflip quote at execution (fetchBestChainflipQuote -> bestQuote) which can change egress amounts vs. what the user confirmed; add a sanity check in the Chainflip branch of SwapService (the doSwap logic) to compare bestQuote.egressAmount (or the fresh bestQuote field) against the originally shown value on params (e.g. params.estimatedEgress or params.originalQuote.egressAmount) and if the relative difference exceeds a small threshold (suggest ~1–3%) log or surface a warning (use processLogger.warn or console.warn) before proceeding with requestDepositAddressV2, including both values and the percent delta so the operator/user can see the deviation. Ensure the check references fetchBestChainflipQuote, bestQuote, and the params field used by the UI and does not block the flow (only logs/warns).
131-143: Chainflip SDK hardcoded to'mainnet'— network mismatch with other AMMs
getThorchainAmmandgetMayachainAmmboth respectwallet.getNetwork(), butgetChainflipSdkis hardcoded to'mainnet'. The Chainflip SDK does support network selection ('mainnet','sisyphos'for staging,'perseverance'for testnet), so this appears intentional rather than a SDK limitation. However, if the suite is used with stagenet/testnet wallets, Chainflip operations will still hit mainnet—creating an inconsistency. Either add a clarifying comment explaining why Chainflip is mainnet-only, or thread the network through from wallet context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/xchain-suite/src/lib/swap/SwapService.ts` around lines 131 - 143, getChainflipSdk is hardcoded to 'mainnet', causing mismatch with AMMs that use wallet.getNetwork(); change getChainflipSdk to accept a network parameter (or thread the wallet network through callers like getChainflipAmm and getMayachainAmm) and pass wallet.getNetwork() into it, mapping your internal network names to Chainflip's supported values ('mainnet', 'sisyphos', 'perseverance') before constructing new SwapSDK; alternatively, if Chainflip must remain mainnet-only, add a clear explanatory comment in getChainflipSdk stating that choice and the reason. Ensure all call sites (e.g., getChainflipAmm) are updated to supply the network arg or rely on the documented mainnet-only behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/xchain-suite/src/components/swap/SwapTrackingModal.tsx`:
- Line 304: Update the tooltip title in SwapTrackingModal to dynamically reflect
whether we're copying a Chainflip deposit channel or a regular transaction hash
by using the conditional on the title prop (check protocol and
depositChannelId); locate the title prop in the component (SwapTrackingModal)
and replace the static string with a conditional expression using protocol ===
'Chainflip' && depositChannelId ? 'Copy deposit channel ID' : 'Copy tx hash' so
the tooltip accurately describes the copied value.
---
Nitpick comments:
In `@tools/xchain-suite/src/lib/swap/SwapService.ts`:
- Line 8: The import mixes type-only and runtime values from
'@xchainjs/xchain-util' which can be confusing: CryptoAmount is imported as a
type and also aliased as CryptoAmountClass for the runtime value, and baseAmount
is aliased to baseAmountFn; add a short inline comment next to the import (or
directly above it) clarifying that CryptoAmount is the type-only alias while
CryptoAmountClass is the runtime class, and that baseAmountFn is the runtime
function alias for baseAmount so future readers understand the dual type/value
usage (refer to CryptoAmount, CryptoAmountClass, baseAmountFn in
SwapService.ts).
- Around line 303-332: The code re-fetches a Chainflip quote at execution
(fetchBestChainflipQuote -> bestQuote) which can change egress amounts vs. what
the user confirmed; add a sanity check in the Chainflip branch of SwapService
(the doSwap logic) to compare bestQuote.egressAmount (or the fresh bestQuote
field) against the originally shown value on params (e.g. params.estimatedEgress
or params.originalQuote.egressAmount) and if the relative difference exceeds a
small threshold (suggest ~1–3%) log or surface a warning (use processLogger.warn
or console.warn) before proceeding with requestDepositAddressV2, including both
values and the percent delta so the operator/user can see the deviation. Ensure
the check references fetchBestChainflipQuote, bestQuote, and the params field
used by the UI and does not block the flow (only logs/warns).
- Around line 131-143: getChainflipSdk is hardcoded to 'mainnet', causing
mismatch with AMMs that use wallet.getNetwork(); change getChainflipSdk to
accept a network parameter (or thread the wallet network through callers like
getChainflipAmm and getMayachainAmm) and pass wallet.getNetwork() into it,
mapping your internal network names to Chainflip's supported values ('mainnet',
'sisyphos', 'perseverance') before constructing new SwapSDK; alternatively, if
Chainflip must remain mainnet-only, add a clear explanatory comment in
getChainflipSdk stating that choice and the reason. Ensure all call sites (e.g.,
getChainflipAmm) are updated to supply the network arg or rely on the documented
mainnet-only behavior.
In `@tools/xchain-suite/src/pages/SwapPage.tsx`:
- Line 86: The filter callback currently types its parameter as any; replace
that with the correct balance type (e.g., Balance from `@xchainjs/xchain-client`)
or an inline type matching the shape { asset: Asset; amount: BaseAmount } so
ESLint no-explicit-any is satisfied; update the predicate (b: Balance) =>
b.asset.chain === asset.chain && b.asset.symbol === asset.symbol (or use the
inline type) and add the appropriate import for Balance if using the exported
type.
- Around line 387-393: The inner redundant call to prices.calculateValue(...)
inside the input overlay should be removed: use the already-computed
inputUsdValue (from earlier in SwapPage) as the single source of truth and
render formatUsdValue(inputUsdValue) directly; update the JSX in SwapPage (the
block that currently checks inputUsdValue !== null && inputUsdValue > 0 and then
re-calls prices.calculateValue) to drop the nested calculateValue check and
simply output ~{formatUsdValue(inputUsdValue)} when inputUsdValue is valid.
This was referenced Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SENTstate breaking Chainflip stage visualization (indexOf returning -1)requestDepositAddressV2fromestimateSwaptodoSwapto avoid premature deposit channel openingtxHashinstead ofdepositChannelIdfor Chainflip swapsgetChainflipSdk()singleton in PoolsPage and SwapTrackingModal instead of repeated instantiationCryptoAmount/baseAmountimportsChainflipBoosttype instead ofas anycastsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements