Conversation
…ds, actors, and TDD tests
…l, Execution, Status)
…d step components Replace 8 useState hooks with xstate machine context. Remove 5 large callback functions and 2 derived useMemos. Wire step components (Input, Quote, Approval, Execution, Status) via state.matches(). Add 6 state-reactive useEffects with ref guards for quoting, approving, executing, polling, completion, and errors.
… components and machine files
The nested @reown/appkit@1.7.2 inside @walletconnect/solana-adapter is missing its dist/ directory, causing Rollup to fail on @reown/appkit/core and @reown/appkit/networks imports. Alias these specific sub-paths to the root @reown/appkit@1.8.17 which has the full dist.
…setId export The pinned npm:@shapeshiftoss/caip@8.16.6 lacks tonAssetId which was added in the local workspace version (8.16.7). Switch to workspace:* to stay in sync with the monorepo and eliminate the SyntaxError at runtime.
…ns with centered layout Remove the QuoteStep as a separate visible step — quoting now happens while InputStep remains visible with a spinner on the swap button. Add min-height step container to prevent layout shifts. Redesign Approval, Execution, and Status (polling/complete/error) screens with centered layout, large themed icon circles, clear typography, and explorer links. All screens use existing CSS custom properties for full dark/light theme support.
…network fee The 'Proceed on ShapeShift' button was disabled for non-EVM/BTC/SOL chains because isButtonDisabled checked for sellAmount/rates which aren't needed for a redirect. Now unsupported chains always enable the button to open app.shapeshift.com. Also adds estimated network fee display (e.g. '0.001 ETH') between the quote selector and the swap button.
Fetch the sell chain native asset USD price alongside sell/buy prices and display the fee as e.g. '0.00123 ETH ($2.45)'. Handles the case where the native asset differs from the sell asset (e.g. USDC on Ethereum).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the swap-widget into an XState-driven, step-based flow; changes UTXO transaction shape from PSBT to deposit-based data; adds SwapWallet context, multi-chain hooks (quoting, approval, execution, polling), new step components, tests, constants, viem chain helpers, styling updates, and test tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Step Components
participant Machine as SwapMachine
participant Wallet as SwapWalletProvider
participant API as Swap API
participant Chain as Blockchain
User->>UI: select assets & amount
UI->>Machine: SET_SELL_ASSET / SET_SELL_AMOUNT
User->>UI: request quote
UI->>Machine: QUOTE_REQUESTED
Machine->>Wallet: read addresses & signers
Machine->>API: getQuote(sell,buy,amount,addresses)
API-->>Machine: quote (may require approval)
alt approval required
Machine->>UI: ApprovalStep
UI->>Wallet: send approval tx
Wallet->>Chain: submit approval
Chain-->>Wallet: txHash
Wallet-->>Machine: APPROVAL_SUCCESS
end
Machine->>UI: ExecutionStep
UI->>Wallet: sign & send swap tx
Wallet->>Chain: submit swap tx
Chain-->>Wallet: txHash
Wallet-->>Machine: EXECUTE_SUCCESS
Machine->>API: poll status(checkTransactionStatus) [interval]
API-->>Machine: status (confirmed/failed)
Machine->>UI: StatusStep (complete or error)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ntext + SwapWalletContext Hooks now consume machine state via SwapMachineCtx.useSelector/useActorRef and wallet state via useSwapWallet() context, dramatically reducing parameter passing. useSwapApproval and useSwapExecution take zero params.
Assets with non-zero balances now appear first, sorted by fiat value descending. Added smooth open/close animations for all modals, step transition fade-in, token list item entry animation, and swap arrow rotation on click.
# Conflicts: # packages/swap-widget/package.json # packages/swap-widget/src/components/SwapWidget.tsx # yarn.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/types/src/api.ts (1)
30-36:⚠️ Potential issue | 🟡 MinorAdding
depositAddressandvaluetoUtxoPsbtTransactionDatablurs the discriminated union boundary.Both
UtxoPsbtTransactionDataandUtxoDepositTransactionDatanow carrydepositAddressandvalue, but only the latter requires them. This weakens the semantic clarity of the union — consumers checkingtype === 'utxo_psbt'may now expect PSBT data but find deposit-style fields instead.If the intent is a fallback path (PSBT sign fails → use depositAddress), consider making this explicit with a dedicated type or a nested optional object rather than flattening overlapping fields onto the PSBT variant.
🤖 Fix all issues with AI agents
In `@packages/public-api/src/routes/quote.ts`:
- Around line 210-216: The public API changed the relay UTXO quote response from
type 'utxo_psbt' to 'utxo_deposit', breaking consumers that check only for
'utxo_psbt'; update the release notes and migration guide to document this
breaking change and the new response shape (type 'utxo_deposit' with
depositAddress, memo, value), and include guidance for consumers to handle both
variants of UtxoTransactionData (or how to detect relay swaps by presence of
relayTransactionMetadata.to) and point to the updated schema in openapi.ts and
the swap-widget handling in useSwapExecution.ts so integrators can update their
type checks and parsing logic accordingly.
In `@packages/swap-widget/package.json`:
- Line 30: Update the new dependencies in package.json to use exact versions (no
caret) so they are pinned: change "@xstate/react": "^5.0.5" to "5.0.5",
"xstate": "^5.28.0" to "5.28.0", "@testing-library/jest-dom": "^6.9.1" to
"6.9.1", "@testing-library/react": "^16.3.2" to "16.3.2", "jsdom": "^28.0.0" to
"28.0.0", "vite-plugin-node-polyfills": "^0.23.0" to "0.23.0", and "vitest":
"^4.0.18" to "4.0.18" so they match the pinned style used for other deps like
"@solana/web3.js", "viem", and "wagmi".
In `@packages/swap-widget/src/components/InputStep.tsx`:
- Around line 214-216: The onChange handler in InputStep.tsx currently uses
e.target.value.replace(/[^0-9.]/g, '') which lets multiple decimal points
through (e.g., "1.2.3") before calling onSellAmountChange and later parseAmount;
change the input sanitization in the onChange before calling onSellAmountChange
to both strip non-digits/non-dots and collapse any extra dots to a single
decimal point (for example, remove non-numeric chars then remove any additional
dots after the first), or alternatively validate and normalize the string (split
on '.', keep the first fractional part only) so that the value passed to
onSellAmountChange/parseAmount contains at most one decimal point.
In `@packages/swap-widget/src/components/SwapWidget.css`:
- Around line 10-12: The current declarations in SwapWidget.css (width,
min-width, max-width) force a fixed 420px box and will overflow on viewports
narrower than 420px; fix it by removing the rigid min-width or making it
responsive: either delete the min-width declaration so the element can shrink,
or add a media query (e.g., `@media` (max-width: 420px)) that resets or lowers
min-width for small screens so width:100% can collapse below 420px; update the
declarations in SwapWidget.css where width, min-width, and max-width are
defined.
In `@packages/swap-widget/src/components/SwapWidget.tsx`:
- Around line 388-395: The useEffect guarded by initialSyncRef prevents reacting
to changes in defaultSellAsset/defaultBuyAsset/defaultSlippage even though they
are listed in the dependency array; decide whether these props are truly
initial-only—if so, remove them from the dependency array and add an explanatory
eslint-disable comment (e.g., above the useEffect add //
eslint-disable-next-line react-hooks/exhaustive-deps -- defaults are
initial-only) so the effect only runs once, or if they must be reactive, remove
the initialSyncRef guard and let the effect call actorRef.send({ type:
'SET_SELL_ASSET', asset: defaultSellAsset }), actorRef.send({ type:
'SET_BUY_ASSET', asset: defaultBuyAsset }), and actorRef.send({ type:
'SET_SLIPPAGE', slippage: defaultSlippage }) whenever
defaultSellAsset/defaultBuyAsset/defaultSlippage change; update tests/docs to
match the chosen behavior.
In `@packages/swap-widget/src/components/TokenSelectModal.tsx`:
- Around line 142-156: visibleRangeAssets is currently sliced from
filteredAssets which misaligns Virtuoso's visibleRange (from rangeChanged)
because the Virtuoso list renders sortedAssets; change visibleRangeAssets to
slice from sortedAssets (not filteredAssets) and move its declaration to after
sortedAssets is computed so it uses the final ordering, then rebuild
assetPrecisions and assetIds from that corrected visibleRangeAssets; ensure
visibleRange (from Virtuoso) remains the dependency for computing the slice so
assetPrecisions and assetIds reflect the same indices Virtuoso renders.
In `@packages/swap-widget/src/demo/App.tsx`:
- Line 379: The demo hardcodes apiBaseUrl='http://0.0.0.0:3001' which can fail
to resolve in some environments; update the usage of the apiBaseUrl prop in
packages/swap-widget/src/demo/App.tsx to default to a client-resolvable host
(e.g., 'http://localhost:3001' or 'http://127.0.0.1:3001') and preferably read
it from an environment variable (e.g., process.env.REACT_APP_API_BASE_URL) with
a safe fallback, ensuring the apiBaseUrl prop passed into the component uses
that env-derived value instead of 0.0.0.0.
In `@packages/swap-widget/src/hooks/useStatusPolling.ts`:
- Around line 66-68: The current branch in useStatusPolling dispatches
actorRef.send({ type: 'STATUS_CONFIRMED' }) for chainType values 'cosmos' and
'other', which silently marks transactions confirmed; instead, change the logic
in useStatusPolling to not auto-confirm for chainType === 'cosmos' or 'other' —
either leave the transaction in a pending state by sending a STATUS_PENDING (or
no status change) and schedule/implement proper polling for those chains, or
send a distinct error/warning status (e.g., STATUS_ERROR or STATUS_WARNING with
a message) so failures aren’t masked; update the conditional that references
chainType and the actorRef.send call (and any STATUS_CONFIRMED usage)
accordingly and add a clear log/warning when polling for cosmos/other is
unimplemented.
In `@packages/swap-widget/src/hooks/useSwapApproval.ts`:
- Around line 85-86: The call in useSwapApproval creates a publicClient with
createPublicClient({ chain, transport: http() }) which relies on chain.rpcUrls
being populated and will fail when chain is the fallback object with empty
rpcUrls; change the transport construction to use an explicit RPC URL (the same
approach used in useSwapExecution) by resolving the chain's RPC endpoint (e.g.,
pick chain.rpcUrls.default.http[0] or your existing helper that returns a valid
URL) and pass http({ url: resolvedRpcUrl }) to createPublicClient before calling
publicClient.waitForTransactionReceipt({ hash: approvalHash }); ensure you
reference createPublicClient, http(), publicClient, waitForTransactionReceipt
and update useSwapApproval accordingly.
- Around line 68-75: The code in useSwapApproval currently constructs
approvalData using BigInt(context.sellAmountBaseUnit ?? '0') which would call
approve(..., 0) when sellAmountBaseUnit is undefined; change the logic to
validate that context.sellAmountBaseUnit exists and parses to a non-zero BigInt
before building approvalData (using encodeFunctionData/approvalData and
quote.approval.spender), and otherwise short-circuit: either throw/return an
error, mark approval as not required, or abort the approval flow so you don't
submit a revoke; ensure the check occurs before the approvalData creation and
surface a clear error or status when sellAmountBaseUnit is missing or zero.
In `@packages/swap-widget/src/hooks/useSwapExecution.ts`:
- Around line 52-57: The fallback chain object assigned to "chain" when
"viemChain" is missing sets rpcUrls.default.http to an empty array which gives
viem no RPC endpoint and will break sendTransaction; update the fallback logic
in useSwapExecution (the const chain = viemChain ?? {...} block) to either 1)
populate rpcUrls.default.http with a valid RPC URL derived from an existing
source (e.g., a configured ENV var, a provider helper, or baseAsset metadata) or
2) explicitly throw or return an error when no RPC is available so
sendTransaction never runs with an empty endpoint; ensure the chosen fix updates
the chain object used by sendTransaction so viem always has at least one valid
RPC URL.
In `@packages/swap-widget/src/hooks/useSwapQuoting.ts`:
- Line 66: The current assignment receiveAddr = effectiveReceiveAddress ||
sendAddress in useSwapQuoting can cause cross-chain funds to be sent to an
address on the wrong chain; change the logic so receiveAddr is never silently
set to sendAddress for cross-chain swaps: require effectiveReceiveAddress when
sellAsset.chain !== buyAsset.chain (or when handling EVM sells) and validate
that effectiveReceiveAddress is compatible with the buyAsset chain before
proceeding to FETCH_QUOTE; if validation fails, return early (or surface an
explicit error) so handleButtonClick / useSwapHandlers (and the FETCH_QUOTE
flow) cannot send a mismatched address to the API. Ensure you reference and
update receiveAddr, effectiveReceiveAddress, sendAddress, useSwapQuoting, and
any validation used by handleButtonClick / FETCH_QUOTE.
In `@packages/swap-widget/src/machines/swapMachine.ts`:
- Around line 246-255: The RETRY transition in the error state always targets
'executing', which is wrong for errors from quoting, approving, or
STATUS_FAILED; add an errorSource string field to the machine context (e.g.,
context.errorSource) and set it when each error occurs (QUOTE_ERROR,
APPROVAL_ERROR, EXECUTE_ERROR, STATUS_FAILED), then change the error.RETRY
transition to route based on that errorSource: if errorSource ===
'EXECUTE_ERROR' target 'executing' (keep guard canRetry and action
incrementRetryCount), if errorSource === 'QUOTE_ERROR' route to 'quoting' (or
prevent retry), if errorSource === 'APPROVAL_ERROR' route to 'approving' (or
prevent retry), and for 'STATUS_FAILED' disallow RETRY (or route to a safe
recovery like 'input'); update any guards/actions to read context.errorSource
and ensure resetSwapState clears errorSource.
In `@packages/swap-widget/src/types/index.ts`:
- Around line 217-220: The type QuoteResponse currently exposes two possible
locations for steps (top-level steps: ApiQuoteStep[] and nested quote?.steps),
causing ambiguity; decide on a single source of truth (either remove the
top-level steps property from QuoteResponse or mark it deprecated and forward it
to quote.steps) and update the type and inline doc comment accordingly so
consumers only reference one consistent field. Specifically, edit the
QuoteResponse definition to either eliminate the top-level steps property
(leaving quote?: { steps?: ApiQuoteStep[] }) or add a clear JSDoc `@deprecated` on
QuoteResponse.steps and ensure any internal code paths (and exported helpers)
read from quote.steps and, if keeping the deprecated field, synchronize it with
quote.steps to avoid drift. Ensure the symbols referenced are QuoteResponse,
steps, quote, quote?.steps, and ApiQuoteStep so reviewers and callers can find
and update usages.
🧹 Nitpick comments (27)
packages/swap-widget/vite.config.ts (1)
53-64: Aliases hardcode internal@reown/appkitdist structure — consider documenting the reason.These paths (
dist/esm/exports/core.js,dist/esm/exports/networks.js) couple the build to specific implementation details of@reown/appkit. While the current version (^1.8.17) works correctly, a dist restructure in future releases could break these aliases. If this alias workaround is necessary due to Vite not respecting@reown/appkit's package.json subpath exports, add a brief comment explaining why the manual alias is needed—this helps maintainers understand whether to update or replace the workaround when bumping versions.packages/swap-widget/src/machines/__tests__/setup.test.ts (1)
1-7: Consider removing this placeholder test.This
expect(true).toBe(true)test provides no value beyond verifying vitest works. If the test infra is already validated by the sibling test files (guards, swapMachine, types), this file is redundant.packages/swap-widget/src/machines/__tests__/guards.test.ts (1)
52-68:hasValidInputtests don't cover missingsellAsset/buyAsset.The guard implementation checks
!!context.sellAsset && !!context.buyAsset, but the tests only exercisesellAmountBaseUnitvariations. Consider adding cases wheresellAssetorbuyAssetis falsy to confirm the guard rejects incomplete input.📝 Suggested additional test cases
it('returns false when sellAmountBaseUnit is empty string', () => { expect(hasValidInput(createTestContext({ sellAmountBaseUnit: '' }))).toBe(false) }) + + it('returns false when sellAsset is missing', () => { + expect(hasValidInput(createTestContext({ sellAsset: undefined as any }))).toBe(false) + }) + + it('returns false when buyAsset is missing', () => { + expect(hasValidInput(createTestContext({ buyAsset: undefined as any }))).toBe(false) + }) })packages/swap-widget/src/components/SwapWidget.css (1)
310-312: The 180° rotation on:activemay cause a jarring snap-back.The
transform: rotate(180deg)is applied only during:activestate. When the user releases the button, it will instantly snap back to 0° (thetransition: transform 0.3s easeon the base rule will animate the return, but the forward rotation happens instantly on press). This might feel janky — consider whether you want the rotation to persist or use a JS-toggled class for a smoother swap animation.packages/swap-widget/src/constants/viemChains.ts (1)
49-68:switchOrAddChainsilently rethrows ambiguous errors whenchainIdis not inVIEM_CHAINS_BY_ID.If
switchChainfails with a "chain not added" error but thechainIdisn't in the local map, the original wallet error is rethrown without context. Consider throwing a more descriptive error to aid debugging.💡 Suggested improvement
if (isChainNotAddedError && chain) { await addChainToWallet(client, chain) await client.switchChain({ id: chainId }) + } else if (isChainNotAddedError && !chain) { + throw new Error(`Chain ${chainId} is not supported by the widget`) } else { throw error }packages/swap-widget/src/machines/guards.ts (2)
14-19: Consider usingfromAssetIdfrom@shapeshiftoss/caipinstead of manual string parsing.The manual
split('/')+split(':')parsing works but is fragile compared to the CAIP utility already used elsewhere (e.g., inuseBalances.ts). If the assetId format ever changes or has edge cases, this could silently break.♻️ Suggested refactor
+import { fromAssetId } from '@shapeshiftoss/caip' + export const isApprovalRequired = (context: SwapMachineContext): boolean => { if (context.quote?.approval?.isRequired !== true || context.chainType !== 'evm') return false - const assetIdParts = context.sellAsset.assetId.split('/') - const namespace = assetIdParts[1]?.split(':')[0] - return namespace === 'erc20' + const { assetNamespace } = fromAssetId(context.sellAsset.assetId) + return assetNamespace === 'erc20' }
21-21: Magic number3for retry limit.Consider extracting to a named constant (e.g.,
MAX_RETRY_COUNT) for clarity and single-source-of-truth if it's referenced elsewhere.packages/public-api/src/docs/openapi.ts (1)
77-83: OptionaldepositAddressandvalueadded toUtxoPsbtTransactionDataSchema— ensure this transitional schema is intentional.Since there's a dedicated
UtxoDepositTransactionDataSchema(lines 85-90) with requireddepositAddressandvalue, having them also as optional on the PSBT schema creates overlap. If the intent is to migrate fromutxo_psbttoutxo_deposit, consider documenting the deprecation path forutxo_psbtto avoid consumer confusion about which schema to expect.packages/swap-widget/src/components/ExecutionStep.tsx (1)
8-8: Unusedcontextandsendprops.Both props are destructured but unused (prefixed with
_). If the component is a placeholder that will wire to the machine later, this is fine. Otherwise, consider narrowing the props to avoid dead code.packages/swap-widget/src/constants/defaults.ts (1)
28-28:POLL_INTERVAL_MSis duplicated —useStatusPolling.tsdefines its own local copy.
useStatusPolling.ts(line 9) redeclaresconst POLL_INTERVAL_MS = 5000instead of importing this constant. One of them should be removed to keep a single source of truth.packages/swap-widget/src/components/StatusStep.tsx (1)
33-38: UTXO explorer URL assumes Bitcoin mainnet only.
mempool.space/tx/won't work for other UTXO chains (Litecoin, Dogecoin, etc.) if the widget ever supports them. For now this is fine if only Bitcoin is in scope, but worth noting for future-proofing. Consider usingsellAsset.explorerTxLinkas the primary source (like you do for EVM) and only falling back to mempool.space.Suggested approach
const explorerUrl = useMemo(() => { if (!txHash) return undefined - if (isSellAssetUtxo) return `https://mempool.space/tx/${txHash}` - if (isSellAssetSolana) return `https://solscan.io/tx/${txHash}` - return `${sellAsset.explorerTxLink ?? 'https://etherscan.io/tx/'}${txHash}` - }, [txHash, isSellAssetUtxo, isSellAssetSolana, sellAsset.explorerTxLink]) + const fallback = isSellAssetUtxo + ? 'https://mempool.space/tx/' + : isSellAssetSolana + ? 'https://solscan.io/tx/' + : 'https://etherscan.io/tx/' + return `${sellAsset.explorerTxLink ?? fallback}${txHash}` + }, [txHash, isSellAssetUtxo, isSellAssetSolana, sellAsset.explorerTxLink])packages/swap-widget/src/hooks/useStatusPolling.ts (1)
98-100: Missing explanation foreslint-disable react-hooks/exhaustive-deps.The coding guidelines require an explanation when disabling the exhaustive-deps rule. This applies to all three effects in this file (lines 99, 118, 132). A brief comment like
// stateValue captures all relevant state transitions; other deps are stable refswould satisfy the guideline. As per coding guidelines: "NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary, and ALWAYS explain why dependencies are excluded."packages/swap-widget/src/components/TokenSelectModal.tsx (1)
175-220: Ref-based sort caching locks the order after the first balance load — late-arriving balances won't re-sort.Once
balanceSortDoneRef.currentis set totrue(Line 217), the sorted result is frozen untilfilteredAssetschanges by reference. If balances trickle in asynchronously (e.g., Bitcoin balance loads after EVM balances), newly loaded balances won't trigger a re-sort because thebalancesdependency change is short-circuited by the ref flag.This may be intentional to avoid list "jumping" during incremental loads. If so, consider adding a brief inline comment explaining the one-shot sort design. If balances are expected to arrive in one batch, this is fine.
packages/swap-widget/src/hooks/useSwapHandlers.ts (1)
17-22: Consider a singleSWAP_TOKENSevent instead of three separate dispatches.Sending three events sequentially works because the machine is in the
inputstate, but it creates three intermediate context snapshots. A single dedicated event (e.g.,SWAP_TOKENS) handled by oneassignaction would be more atomic and efficient, avoiding intermediate state wheresellAssetandbuyAssetare briefly identical.packages/swap-widget/src/hooks/useSwapExecution.ts (2)
19-226: Mixed context access: uses both render-timecontextand snapshotactorRef.getSnapshot()— pick one for consistency.Line 20-21 reads the snapshot from
actorRef.getSnapshot(), but Lines 26-161 read from the render-timecontext(Line 12). While this works because the state transition that sets the quote occurs before the re-render, mixing the two patterns is confusing and fragile. If additional events are processed between render and effect execution, the render-timecontextcould become stale.♻️ Suggested approach: use snapshot consistently
useEffect(() => { const snap = actorRef.getSnapshot() if (!snap.matches('executing') || executingRef.current) return executingRef.current = true const executeSwap = async () => { try { - const quote = context.quote + const { context: ctx } = snap + const quote = ctx.quote if (!quote) { actorRef.send({ type: 'EXECUTE_ERROR', error: 'No quote available' }) return } - if (context.isSellAssetEvm) { + if (ctx.isSellAssetEvm) { // ... use ctx throughoutThis same pattern applies to
useSwapApprovalanduseSwapQuoting— consider aligning all three hooks.
225-226: Missing explanation foreslint-disable-next-line react-hooks/exhaustive-deps.Per coding guidelines, an
eslint-disablefor hooks exhaustive-deps must include an explanation of why dependencies are excluded. Consider adding a comment explaining thatstateValueis the intentional sole trigger and that other values are read from the snapshot at execution time.As per coding guidelines: "NEVER use
// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary, and ALWAYS explain why dependencies are excluded if using eslint disable"packages/swap-widget/src/hooks/useSwapApproval.ts (1)
98-99: Missing explanation for eslint-disable (same pattern asuseSwapExecution).packages/swap-widget/src/hooks/useSwapQuoting.ts (1)
8-13:BalanceDatatype is defined locally — consider importing fromuseBalancesto avoid duplication.packages/swap-widget/src/machines/swapMachine.ts (2)
163-166: Theidlestate is a pass-through — consider usinginitial: 'input'directly.The
idlestate immediately transitions toinputviaalways. Unless you plan to add entry actions or conditions inidle, this adds an unnecessary state.
41-53:isApprovalRequiredguard is inlined rather than delegating toguardFnslike all other guards.Other guards (Lines 47, 54-60) consistently delegate to
guardFns.someGuard(context). TheisApprovalRequiredguard on Lines 48-53 contains inline logic that reads from bothcontextandevent. Consider extracting this to theguardsmodule for consistency and testability.packages/swap-widget/src/components/InputStep.tsx (1)
49-88: Three destructured props are underscore-prefixed and unused:send,enableWalletConnection,onConnectWallet.These add noise to the component's interface. If they're intended for future use, consider removing them from the props type until needed (YAGNI). If they're used by other callers but not this component, the type should be narrowed.
packages/swap-widget/src/components/SwapWidget.tsx (6)
69-70:useSelector(s => s)subscribes to the entire state snapshot, causing re-renders on every machine transition.This component will re-render whenever any field in the machine context or state value changes, even when only unrelated parts update. Since
SwapWidgetContentis large and renders step components and modals, this can be costly.Consider selecting only the fields you need, or at minimum selecting
state.valueandstate.contextseparately with targeted selectors per step.♻️ Suggested approach
- const state = SwapMachineCtx.useSelector(s => s) - const actorRef = SwapMachineCtx.useActorRef() + const actorRef = SwapMachineCtx.useActorRef() + const stateValue = SwapMachineCtx.useSelector(s => s.value) + const context = SwapMachineCtx.useSelector(s => s.context)Or better yet, use fine-grained selectors with equality checks for each step's required data so that unrelated transitions don't cause cascading re-renders.
165-169: Inline arrow functions passed as props to child components are not memoized.Lines 167, 274, 285, and 292 all create new function references each render. Per coding guidelines, event handlers passed as props should be wrapped in
useCallback. Consider extracting them alongsideopenAddressModal(line 154) for consistency.♻️ Example
const openAddressModal = useCallback(() => setIsAddressModalOpen(true), []) + const closeAddressModal = useCallback(() => setIsAddressModalOpen(false), []) + const openSettings = useCallback(() => setIsSettingsOpen(true), []) + const closeSettings = useCallback(() => setIsSettingsOpen(false), []) + const closeTokenModal = useCallback(() => setTokenModalType(null), [])As per coding guidelines: "ALWAYS use
useCallbackfor event handlers and functions passed as props".
313-314: Unsafeas WalletClientcast fromunknown.
walletClientis typedunknown(line 314) and later cast withas WalletClient(line 359) without any runtime validation. If an external consumer passes a non-Viem wallet client, this silently producesundefinedfrom.account?.addressat best, or throws at worst.Consider adding a type guard or at minimum a truthiness + shape check:
♻️ Suggested approach
const walletAddress = useMemo(() => { if (!walletClient) return undefined - return (walletClient as WalletClient).account?.address + if ( + typeof walletClient === 'object' && + walletClient !== null && + 'account' in walletClient + ) { + return (walletClient as WalletClient).account?.address + } + return undefined }, [walletClient])As per coding guidelines: "ALWAYS use type guards instead of type assertions when possible in TypeScript" and "NEVER use type assertions without proper validation in TypeScript".
Also applies to: 357-360
470-471: Fresh[]references created on every render when props are nullish.
props.disabledAssetIds ?? []andprops.disabledChainIds ?? []produce new array references each render when the props are not provided. These propagate as changed props to child components. Extract a module-level constant to avoid unnecessary re-renders:♻️ Suggested fix
+const EMPTY_ARRAY: string[] = [] + // Then use: - disabledAssetIds={props.disabledAssetIds ?? []} - disabledChainIds={props.disabledChainIds ?? []} + disabledAssetIds={props.disabledAssetIds ?? EMPTY_ARRAY} + disabledChainIds={props.disabledChainIds ?? EMPTY_ARRAY}Apply the same fix in
SwapWidgetWithInternalWallet(lines 506-507).
447-515: Consider extracting duplicatedSwapWidgetCoreprop-passing.
SwapWidgetWithExternalWalletandSwapWidgetWithInternalWalletboth pass ~15 identical props toSwapWidgetCore. A shared helper (or a props-builder function) could reduce this duplication and prevent future drift if new props are added to one but missed in the other.
453-476: No error boundary wrapping the widget tree.If any child component throws during render, the entire host application could crash. Consider wrapping
SwapWidgetCore(or theSwapMachineCtx.Providersubtree) in an error boundary with a user-friendly fallback, especially since this is an embeddable widget consumed by third-party apps.This applies to both
SwapWidgetWithExternalWalletandSwapWidgetWithInternalWallet.As per coding guidelines: "ALWAYS wrap React components in error boundaries and provide user-friendly fallback components with error logging".
NeOMakinG
force-pushed
the
widget-polish
branch
from
7aceaea to
2909093
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swap-widget/src/components/SwapWidget.tsx (1)
448-529:⚠️ Potential issue | 🟠 MajorWrap SwapWidget rendering in an error boundary to prevent render errors from tearing down the entire widget.
Currently, any render error in SwapWidgetCore or its children will crash the widget without a fallback. Add a user-friendly error boundary at the top level, following the project's established pattern (e.g., using a fallback component with retry capability and error logging). This requires adding
react-error-boundaryto the package dependencies.
🤖 Fix all issues with AI agents
In `@packages/swap-widget/src/hooks/useSwapQuoting.ts`:
- Around line 55-63: The current sendAddress assignment falls through to
solanaAddress for any non‑EVM/UTXO sell chain; update the guard to explicitly
check for Solana (e.g. context.isSellAssetSolana) and only use solanaAddress
when that flag is true, and otherwise call actorRef.send({ type: 'QUOTE_ERROR',
error: 'Unsupported sell chain for quoting' }) and return; change the ternary
that defines sendAddress (and keep the existing actorRef.send call used for
missing address) so unsupported chains are rejected rather than defaulting to
solanaAddress.
In `@packages/swap-widget/src/machines/types.ts`:
- Around line 3-20: Replace the string union ErrorSource and the inline union
for chainType with exported string enums (e.g., export enum ErrorSource {
QUOTE_ERROR = 'QUOTE_ERROR', APPROVAL_ERROR = 'APPROVAL_ERROR', EXECUTE_ERROR =
'EXECUTE_ERROR', STATUS_FAILED = 'STATUS_FAILED' } and export enum ChainType {
EVM = 'evm', UTXO = 'utxo', SOLANA = 'solana', COSMOS = 'cosmos', OTHER =
'other' }) and update SwapMachineContext to use ErrorSource | null for
errorSource and ChainType for chainType (e.g., chainType: ChainType). Also scan
for other references to the old union types and update their type
annotations/usages to the new enums.
🧹 Nitpick comments (4)
packages/swap-widget/src/machines/__tests__/guards.test.ts (1)
53-69: Consider adding coverage for missingsellAsset/buyAsset.
hasValidInputalso checks!!context.sellAssetand!!context.buyAsset, but these paths aren't exercised. Adding a couple of cases (e.g.,sellAsset: undefined as any,buyAsset: undefined as any) would strengthen the suite.packages/swap-widget/src/types/index.ts (1)
183-189: Consider extracting the inline Solana instruction type to a named type alias.This inline type is fairly complex and will likely be referenced by consumers or execution logic. Extracting it would improve reusability and readability.
♻️ Suggested refactor
+export type SolanaInstructionData = { + programId: string + keys: { pubkey: string; isSigner: boolean; isWritable: boolean }[] + data: { data: number[] } +} + export type ApiQuoteStep = { // ... - solanaTransactionMetadata?: { - instructions: { - programId: string - keys: { pubkey: string; isSigner: boolean; isWritable: boolean }[] - data: { data: number[] } - }[] - } + solanaTransactionMetadata?: { + instructions: SolanaInstructionData[] + } }packages/swap-widget/src/hooks/useSwapExecution.ts (1)
107-153: Refactor to avoidletwith a const and inline async IIFE.The conditional logic assigning
txidshould useconstwith an async IIFE instead ofletwith multiple assignments, following the project guideline to avoidletfor conditional logic.Suggested refactor
- let txid: string - - if (transactionData.type === 'utxo_psbt') { - try { - const isHex = /^[0-9a-fA-F]+$/.test(transactionData.psbt) - const psbtBase64 = isHex - ? Buffer.from(transactionData.psbt, 'hex').toString('base64') - : transactionData.psbt - - txid = await bitcoin.signPsbt({ - psbt: psbtBase64, - signInputs: [], - broadcast: true, - }) - } catch (psbtError) { - const msg = - psbtError instanceof Error ? psbtError.message.toLowerCase() : String(psbtError) - const isUserRejection = - msg.includes('rejected') || - msg.includes('denied') || - msg.includes('cancelled') || - msg.includes('user refused') - - if (isUserRejection) throw psbtError - - if (!transactionData.depositAddress) throw psbtError - - const step = outerStep ?? innerStep - txid = await bitcoin.sendTransfer({ - recipientAddress: transactionData.depositAddress, - amount: - transactionData.value ?? - step?.sellAmountCryptoBaseUnit ?? - quote.sellAmountCryptoBaseUnit, - ...(transactionData.opReturnData && { memo: transactionData.opReturnData }), - }) - } - } else if (transactionData.type === 'utxo_deposit') { - txid = await bitcoin.sendTransfer({ - recipientAddress: transactionData.depositAddress, - amount: transactionData.value, - memo: transactionData.memo, - }) - } else { - throw new Error(`Unsupported UTXO transaction type: ${transactionData.type}`) - } + const txid = await (async () => { + if (transactionData.type === 'utxo_psbt') { + try { + const isHex = /^[0-9a-fA-F]+$/.test(transactionData.psbt) + const psbtBase64 = isHex + ? Buffer.from(transactionData.psbt, 'hex').toString('base64') + : transactionData.psbt + + return await bitcoin.signPsbt({ + psbt: psbtBase64, + signInputs: [], + broadcast: true, + }) + } catch (psbtError) { + const msg = + psbtError instanceof Error ? psbtError.message.toLowerCase() : String(psbtError) + const isUserRejection = + msg.includes('rejected') || + msg.includes('denied') || + msg.includes('cancelled') || + msg.includes('user refused') + + if (isUserRejection) throw psbtError + if (!transactionData.depositAddress) throw psbtError + + const step = outerStep ?? innerStep + return await bitcoin.sendTransfer({ + recipientAddress: transactionData.depositAddress, + amount: + transactionData.value ?? + step?.sellAmountCryptoBaseUnit ?? + quote.sellAmountCryptoBaseUnit, + ...(transactionData.opReturnData && { memo: transactionData.opReturnData }), + }) + } + } + if (transactionData.type === 'utxo_deposit') { + return await bitcoin.sendTransfer({ + recipientAddress: transactionData.depositAddress, + amount: transactionData.value, + memo: transactionData.memo, + }) + } + throw new Error(`Unsupported UTXO transaction type: ${transactionData.type}`) + })()packages/swap-widget/src/components/SwapWidget.tsx (1)
165-299: Memoize inline handlers and array props passed to child components.Several inline callbacks and array literals are recreated each render; use
useCallback/useMemoper project guidance.♻️ Suggested update
const SwapWidgetContent = ({ @@ }: SwapWidgetContentProps) => { @@ - const openAddressModal = useCallback(() => setIsAddressModalOpen(true), []) + const openAddressModal = useCallback(() => setIsAddressModalOpen(true), []) + const closeAddressModal = useCallback(() => setIsAddressModalOpen(false), []) + const closeTokenModal = useCallback(() => setTokenModalType(null), []) + const openSettings = useCallback(() => setIsSettingsOpen(true), []) + const closeSettings = useCallback(() => setIsSettingsOpen(false), []) + const currentAssetIds = useMemo( + () => [state.context.sellAsset.assetId, state.context.buyAsset.assetId], + [state.context.sellAsset.assetId, state.context.buyAsset.assetId], + ) @@ - onClick={() => setIsSettingsOpen(true)} + onClick={openSettings} @@ - onClose={() => setTokenModalType(null)} + onClose={closeTokenModal} @@ - currentAssetIds={[state.context.sellAsset.assetId, state.context.buyAsset.assetId]} + currentAssetIds={currentAssetIds} @@ - onClose={() => setIsSettingsOpen(false)} + onClose={closeSettings} @@ - onClose={() => setIsAddressModalOpen(false)} + onClose={closeAddressModal}As per coding guidelines: ALWAYS use useMemo for expensive computations, object/array creations, and filtered data; and ALWAYS use useCallback for event handlers and functions passed as props.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/swap-widget/src/hooks/useSwapQuoting.ts (2)
8-13:balanceFormattedinBalanceDatais declared but never consumed.Only
balanceis accessed inside the hook (Lines 40–41). Callers are forced to providebalanceFormattedfor no benefit, widening the public API surface unnecessarily.♻️ Proposed fix: remove unused field
type BalanceData = | { balance: string - balanceFormatted: string } | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/hooks/useSwapQuoting.ts` around lines 8 - 13, The BalanceData type declares an unused field balanceFormatted which is never read by useSwapQuoting (only balance is accessed); remove balanceFormatted from the BalanceData union so callers are not forced to provide it and adjust any references to BalanceData (e.g., type alias BalanceData and any parameters/returns of useSwapQuoting that use it) to only include balance or undefined.
94-96: Eslint suppression comment misstates why deps are omitted.The comment reads "other deps are stable refs read from snapshot", but
walletAddress,effectiveReceiveAddress,bitcoinAddress,solanaAddress,rates,sellAssetBalance,context, andapiClientare all reactive values sourced from React hooks and props — not stable refs. The actual intent is that the effect is intentionally state-machine-driven (fires only on state transitions), so stale closures are an accepted trade-off. The inaccurate description could mislead future maintainers into believing these deps are safe to omit for a different reason.♻️ Proposed fix: accurate suppression comment
- // eslint-disable-next-line react-hooks/exhaustive-deps -- stateValue is the sole trigger; other deps are stable refs read from snapshot + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally state-machine-driven: effect fires only on 'quoting' state transitions; other deps are captured at transition time🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/hooks/useSwapQuoting.ts` around lines 94 - 96, The eslint suppression comment on the effect that calls fetchQuote() is misleading — it incorrectly claims omitted deps are "stable refs read from snapshot"; instead update the comment to clearly state that the effect is intentionally state-machine-driven (fires only on stateValue transitions) and that stale closures for walletAddress, effectiveReceiveAddress, bitcoinAddress, solanaAddress, rates, sellAssetBalance, context, and apiClient are an accepted trade-off; keep the dependency array as [stateValue] and ensure the comment references fetchQuote and stateValue so future maintainers understand why other reactive values are omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swap-widget/src/components/TokenSelectModal.tsx`:
- Around line 182-223: sortedAssets currently mutates prevFilteredRef,
balanceSortDoneRef, and sortedAssetsRef inside useMemo (violating purity) and
fails to set balanceSortDoneRef when balances is empty; instead, make sorting
pure by removing ref writes from the useMemo and drive the sorted list via
useState (e.g., const [sortedAssetsState, setSortedAssetsState]) plus a
useEffect that watches [filteredAssets, balances, marketData]; in that effect
compute the sorted result (separate withBalance and withoutBalance, sort by
fiatValue) and call setSortedAssetsState(result), and manage a separate
loaded/balanceSortDone state (e.g., balanceSortDone boolean via useState) that
you set to true even when balances is empty to prevent repeated recomputation;
replace usages of sortedAssetsRef.current/prevFilteredRef/balanceSortDoneRef
with the new state values and keep the useMemo pure or remove it entirely for
sortedAssets.
In `@packages/swap-widget/src/hooks/useBitcoinSigning.ts`:
- Around line 11-15: The SignInput type is currently unexported but used in the
exported SignPsbtParams.signInputs: SignInput[]; export the type so callers can
import it. Update the declaration for SignInput (the type alias named
"SignInput") to be exported (add the export modifier) so external code can
reference it when constructing SignPsbtParams.signInputs, then run the
TypeScript build/typecheck to ensure no other public types are missing exports.
In `@packages/swap-widget/src/hooks/useSwapQuoting.ts`:
- Line 48: Validate context.slippage before converting: in useSwapQuoting (where
slippageDecimal is computed from context.slippage) check that context.slippage
is a non-empty numeric string and that Number.isFinite(parsed =
parseFloat(context.slippage)); if the value is invalid, set the quote error
state (the same QUOTE_ERROR path used elsewhere) or return early instead of
calling apiClient.getQuote, and only compute slippageDecimal =
(parsed/100).toString() when parsed is valid; ensure any downstream callers of
slippageTolerancePercentageDecimal receive a validated string or the flow
short-circuits with an error.
---
Nitpick comments:
In `@packages/swap-widget/src/hooks/useSwapQuoting.ts`:
- Around line 8-13: The BalanceData type declares an unused field
balanceFormatted which is never read by useSwapQuoting (only balance is
accessed); remove balanceFormatted from the BalanceData union so callers are not
forced to provide it and adjust any references to BalanceData (e.g., type alias
BalanceData and any parameters/returns of useSwapQuoting that use it) to only
include balance or undefined.
- Around line 94-96: The eslint suppression comment on the effect that calls
fetchQuote() is misleading — it incorrectly claims omitted deps are "stable refs
read from snapshot"; instead update the comment to clearly state that the effect
is intentionally state-machine-driven (fires only on stateValue transitions) and
that stale closures for walletAddress, effectiveReceiveAddress, bitcoinAddress,
solanaAddress, rates, sellAssetBalance, context, and apiClient are an accepted
trade-off; keep the dependency array as [stateValue] and ensure the comment
references fetchQuote and stateValue so future maintainers understand why other
reactive values are omitted.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/swap-widget/src/constants/viemChains.ts (2)
56-62: Type assertion without validation — use a type guard instead
error as { code?: number; message?: string }asserts a type without validating the runtime shape. Per the project coding guidelines, type assertions require proper validation. Useinstanceoffor known error types or a narrowing check:♻️ Proposed fix using a type guard
+const isRpcError = (err: unknown): err is { code?: number; message?: string } => + typeof err === 'object' && err !== null export const switchOrAddChain = async (client: WalletClient, chainId: number): Promise<void> => { const chain = VIEM_CHAINS_BY_ID[chainId] try { await client.switchChain({ id: chainId }) } catch (error) { - const switchError = error as { code?: number; message?: string } + const switchError = isRpcError(error) ? error : {} const isChainNotAddedError = switchError.code === 4902 || switchError.message?.toLowerCase().includes('unrecognized chain') || switchError.message?.toLowerCase().includes('chain not added') || switchError.message?.toLowerCase().includes('try adding the chain')As per coding guidelines: "ALWAYS use type guards instead of type assertions when possible" and "NEVER use type assertions without proper validation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/constants/viemChains.ts` around lines 56 - 62, The catch block currently uses a raw type assertion `error as { code?: number; message?: string }` to create `switchError` and then reads `switchError.code` and `switchError.message`; replace this with a runtime type guard (e.g., an `isErrorWithCodeAndMessage(err): err is { code?: number; message?: string }` or checks using `err instanceof Error` plus `typeof (err as any).code === 'number'`) and only compute `isChainNotAddedError` after the guard passes so you safely access `.code` and `.message` without a bare assertion; update the code around the catch block and the `isChainNotAddedError` check to use that guard.
34-49: Use viem'saddChaininstead of manualwallet_addEthereumChainrequestViem's
WalletClientprovides a first-classaddChainaction that handles the fullwallet_addEthereumChainrequest internally. The current implementation manually constructs all params (hex conversion,rpcUrls.default.httpextraction, optionalblockExplorerUrlshandling), which is redundant and fragile.Refactor to use `addChain`
-export const addChainToWallet = async (client: WalletClient, chain: Chain): Promise<void> => { - const { id, name, nativeCurrency, rpcUrls, blockExplorers } = chain - - await client.request({ - method: 'wallet_addEthereumChain', - params: [ - { - chainId: `0x${id.toString(16)}`, - chainName: name, - nativeCurrency, - rpcUrls: rpcUrls.default.http, - blockExplorerUrls: blockExplorers?.default ? [blockExplorers.default.url] : undefined, - }, - ], - }) -} +export const addChainToWallet = async (client: WalletClient, chain: Chain): Promise<void> => { + await client.addChain({ chain }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/constants/viemChains.ts` around lines 34 - 49, The addChainToWallet function is manually building and sending a wallet_addEthereumChain request; replace that with viem's built-in addChain action on WalletClient. Update addChainToWallet to call the WalletClient's addChain method (e.g., await client.addChain(chain)) passing the Chain object directly, remove the manual hex conversion/rpcUrls/blockExplorerUrls construction and the client.request call, and ensure the function still accepts (client: WalletClient, chain: Chain) and returns Promise<void>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/swap-widget/src/constants/viemChains.ts`:
- Around line 56-62: The catch block currently uses a raw type assertion `error
as { code?: number; message?: string }` to create `switchError` and then reads
`switchError.code` and `switchError.message`; replace this with a runtime type
guard (e.g., an `isErrorWithCodeAndMessage(err): err is { code?: number;
message?: string }` or checks using `err instanceof Error` plus `typeof (err as
any).code === 'number'`) and only compute `isChainNotAddedError` after the guard
passes so you safely access `.code` and `.message` without a bare assertion;
update the code around the catch block and the `isChainNotAddedError` check to
use that guard.
- Around line 34-49: The addChainToWallet function is manually building and
sending a wallet_addEthereumChain request; replace that with viem's built-in
addChain action on WalletClient. Update addChainToWallet to call the
WalletClient's addChain method (e.g., await client.addChain(chain)) passing the
Chain object directly, remove the manual hex
conversion/rpcUrls/blockExplorerUrls construction and the client.request call,
and ensure the function still accepts (client: WalletClient, chain: Chain) and
returns Promise<void>.
0xApotheosis
left a comment
0xApotheosis
left a comment
There was a problem hiding this comment.
✅ Confirmed all API still work as expected
❌ Could not get transactions to send to WC wallet. Nothing happened with clicking "Swap", it just spun on the screen asking to confirm the TX in the wallet
❌ Also noticed if you refresh a page while it's connected to a WalletConnect wallet, it gets in a really bad state. It does not allow disconnection, but its also not properly connected.
✅ 1. EVM swap: Connect EVM wallet via AppKit → swap ETH→USDC → should execute via sendTransaction
2. Bitcoin swap: Connect Bitcoin wallet (Leather/Phantom) via AppKit → swap BTC→ETH → should use sendTransfer to Relay deposit address (not PSBT)
3. Solana swap: Connect Solana wallet via AppKit → swap SOL→USDC → should build and send transaction from instruction data
4. BTC address reactivity: Connect Bitcoin wallet → switch accounts in Reown modal → widget FROM address and balance should update immediately
5. Balance filtering: Select BTC as sell asset → should show BTC balance only (not BCH/LTC/DOGE)
✅ 6. Public API: POST /quote with BTC sell asset + Relay swapper → response should have transactionData.type === 'utxo_deposit' (not utxo_psbt)
NeOMakinG
force-pushed
the
widget-polish
branch
from
ad2a25c to
e416445
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swap-widget/src/types/index.ts (1)
284-287:⚠️ Potential issue | 🟡 Minor
getEvmNetworkIdreturnsNaNfor non-numeric chain references without any safeguard.
parseInt(chainReference, 10)silently producesNaNifchainReferenceisn't a valid number. Callers using the result for network switching or RPC calls could hit unexpected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/types/index.ts` around lines 284 - 287, getEvmNetworkId currently does parseInt(chainReference, 10) which yields NaN for non-numeric chain references; update getEvmNetworkId to validate chainReference (from fromChainId(chainId as ChainId)) before parsing—e.g., ensure it matches a numeric pattern or Number.isFinite(Number(chainReference))—and then either return the parsed integer or throw a descriptive Error (or return a documented fallback) so callers won't receive NaN when switching networks or calling RPCs.
♻️ Duplicate comments (3)
packages/swap-widget/src/components/TokenSelectModal.tsx (1)
211-252: Ref mutations insideuseMemoremain — previously flagged.The
sortedAssetsmemo still mutatesprevFilteredRef,balanceSortDoneRef, andsortedAssetsRefas side effects. This was raised in a prior review cycle and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/TokenSelectModal.tsx` around lines 211 - 252, The useMemo for sortedAssets currently performs side effects by mutating prevFilteredRef, balanceSortDoneRef, and sortedAssetsRef; make sortedAssets a pure computation (no ref writes) and move the ref updates into a separate useEffect that reacts to changes in filteredAssets, balances, and marketData. Specifically, have useMemo only compute and return the sorted array (reference sortedAssetsMemo), and add a useEffect that compares prevFilteredRef.current to filteredAssets, sets balanceSortDoneRef.current and sortedAssetsRef.current as needed, and updates prevFilteredRef.current—referencing the sortedAssets variable created by useMemo and the existing refs (prevFilteredRef, balanceSortDoneRef, sortedAssetsRef) and functions (bnOrZero) to locate where to change behavior.packages/swap-widget/src/hooks/useBitcoinSigning.ts (1)
11-15:SignInputis still unexported despite being part of the public API surface.
SignPsbtParams(exported at line 36) referencesSignInput[]for itssignInputsfield, so callers constructing this param shape cannot import the type by name. The AI summary describes this as an "Added public type" but theexportkeyword is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/hooks/useBitcoinSigning.ts` around lines 11 - 15, The SignInput type is used in the public SignPsbtParams signature but is not exported; update the declaration of SignInput to be exported (export type SignInput = { address: string; index: number; sighashTypes?: number[] }) so callers can import it, and verify that SignPsbtParams still references SignInput[] for its signInputs field; ensure the exported name matches any places referencing SignInput.packages/swap-widget/src/components/SwapWidget.css (1)
15-17:min-width: 420pxis still present — will overflow on viewports narrower than 420px.This was previously flagged and confirmed as fixed, but the constraint remains in the latest code. With
width: 100%; min-width: 420px; max-width: 420px, the widget is a rigid 420px box that causes horizontal scrolling on mobile screens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/SwapWidget.css` around lines 15 - 17, The CSS block in SwapWidget.css currently forces a rigid 420px layout with "width: 100%; min-width: 420px; max-width: 420px" which breaks on narrow viewports; remove or change the "min-width: 420px" declaration inside the SwapWidget.css width block (the same declarations shown) so the widget can shrink on mobile—leave "width: 100%" and "max-width: 420px" (or set a responsive min-width like min-width: 0) to prevent horizontal overflow and ensure the component adapts to viewports under 420px.
🧹 Nitpick comments (12)
packages/swap-widget/src/components/TokenSelectModal.tsx (3)
95-104: Consider convertingallowedAssetIdsto aSetfor O(1) lookups.
allowedAssetIds.includes()is called once per asset in bothallowedAssetChainIds(Line 99) andfilteredAssets(Line 130). With a large asset list and a non-trivialallowedAssetIds, this is O(n×m). A singleSetderived from the prop avoids this:♻️ Suggested refactor
+ const allowedAssetIdSet = useMemo( + () => (allowedAssetIds ? new Set(allowedAssetIds) : undefined), + [allowedAssetIds], + ) + const allowedAssetChainIds = useMemo(() => { - if (!allowedAssetIds || allowedAssetIds.length === 0) return undefined + if (!allowedAssetIdSet || allowedAssetIdSet.size === 0) return undefined const chainIds = new Set<ChainId>() for (const asset of allAssets) { - if (allowedAssetIds.includes(asset.assetId)) { + if (allowedAssetIdSet.has(asset.assetId)) { chainIds.add(asset.chainId) } } return Array.from(chainIds) - }, [allowedAssetIds, allAssets]) + }, [allowedAssetIdSet, allAssets])And similarly in
filteredAssets:- if (allowedAssetIds && allowedAssetIds.length > 0) { - assets = assets.filter(asset => allowedAssetIds.includes(asset.assetId)) + if (allowedAssetIdSet && allowedAssetIdSet.size > 0) { + assets = assets.filter(asset => allowedAssetIdSet.has(asset.assetId)) }Also applies to: 129-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/TokenSelectModal.tsx` around lines 95 - 104, Convert allowedAssetIds into a Set once and use O(1) lookups in both the allowedAssetChainIds and filteredAssets computations: create a memoized allowedAssetIdSet (using useMemo) derived from allowedAssetIds, update the allowedAssetChainIds logic to call allowedAssetIdSet.has(asset.assetId) instead of allowedAssetIds.includes(...), and similarly replace includes with .has in the filteredAssets calculation; ensure the new memo depends on allowedAssetIds and then use that Set in both allowedAssetChainIds and filteredAssets to avoid repeated O(n×m) scans.
456-468: Fiat value is computed twice — once during sort and again during render.The
bnOrZero(balance).div(...).times(price)calculation at lines 460-467 duplicates the same math done insidesortedAssetsat lines 234-238. Consider storing the computed fiat values in aMap<AssetId, number>alongside the sort, then referencing it during render to avoid redundant BigNumber arithmetic on every visible row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/TokenSelectModal.tsx` around lines 456 - 468, The fiat value calculation is duplicated: the BigNumber math done in sortedAssets (referenced as sortedAssets) is recomputed in the render block using bnOrZero(balance.balance).div(...).times(marketData[asset.assetId].price); fix by computing fiat values once when building sortedAssets (or immediately after) and store them in a lookup (e.g., Map<assetId, number> or an object keyed by asset.assetId), then replace the inline BigNumber expression in the TokenSelectModal render with a simple lookup (using marketData and the precomputed map) to avoid repeating heavy bnOrZero math on every visible row.
417-478: Consider extractingitemContentinto a memoized component oruseCallback.The inline
itemContentrender function (60+ lines) is recreated on every render and captures many dependencies. Extracting it into a separateTokenRowcomponent wrapped withmemowould prevent re-renders for rows whose props haven't changed, improving scroll performance — especially since balance/market data loads asynchronously and triggers re-renders.As per coding guidelines: "ALWAYS use
useCallbackfor event handlers and functions passed as props."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/TokenSelectModal.tsx` around lines 417 - 478, The inline itemContent render function in TokenSelectModal is recreated every render and causes unnecessary row re-renders; extract it into a separate memoized component (e.g., TokenRow) that accepts props needed for rendering (asset, balance, marketData[asset.assetId], chainInfoMap.get(asset.chainId), loadingAssetIds.has(asset.assetId), walletAddress/bitcoinAddress/solanaAddress, handleAssetSelect, bnOrZero) and wrap that component with React.memo (and useCallback for any handlers like onClick pointing to handleAssetSelect) so rows only re-render when their specific props change; replace the inline itemContent with a stable function that returns <TokenRow ... /> to improve scroll performance.packages/swap-widget/src/machines/guards.ts (1)
21-21: Consider extracting the retry limit to a named constant.The magic number
3would be clearer as a constant (e.g.MAX_RETRY_COUNT), making it easier to adjust and self-documenting.As per coding guidelines, "Use UPPER_SNAKE_CASE for constants and configuration values with descriptive names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/machines/guards.ts` at line 21, Extract the magic number 3 into a named constant (e.g., MAX_RETRY_COUNT) and update the guard to use it: change the comparison in canRetry (which accepts SwapMachineContext) to reference MAX_RETRY_COUNT so the retry limit is centralized, follows UPPER_SNAKE_CASE, and is easier to adjust and document.packages/swap-widget/src/components/ExecutionStep.tsx (1)
8-8: Unusedcontextandsendprops are underscore-aliased rather than removed from the type.Both props are destructured as
_contextand_sendbut never referenced. If the shared step interface requires them for consistency across steps, consider at minimum omitting the destructuring and using_props: ExecutionStepPropsor just_: ExecutionStepPropsso the intent is clearer. Otherwise, narrow the props type for this component.As per coding guidelines, "When function parameters are unused due to interface requirements, refactor the interface or implementation to remove them rather than prefixing with underscore".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/ExecutionStep.tsx` at line 8, The ExecutionStep component currently destructures unused props as _context and _send from ExecutionStepProps; either stop destructuring them and accept a single unused prop (e.g., _props: ExecutionStepProps or _: ExecutionStepProps) to show intent, or narrow the component signature to only the required props by removing context and send from the parameter type; update the ExecutionStep declaration (symbol ExecutionStep) to one of these two options and remove the unused identifiers _context and _send accordingly so the signature matches actual usage or the shared interface requirements.packages/swap-widget/src/components/WalletProvider.tsx (1)
61-62: Consider memoizing derived wallet state.
connectedAddressandisConnectedare recomputed on every render. While cheap, the coding guidelines specifyuseMemofor derived values. A simpleuseMemokeeps the component consistent with the rest of the codebase.♻️ Suggested memoization
- const connectedAddress = walletAddress ?? appKitAddress - const isConnected = !!walletAddress || appKitConnected + const connectedAddress = useMemo(() => walletAddress ?? appKitAddress, [walletAddress, appKitAddress]) + const isConnected = useMemo(() => !!walletAddress || appKitConnected, [walletAddress, appKitConnected])You'd also need to add
useMemoto the import on line 4.As per coding guidelines, "ALWAYS use
useMemofor derived values and computed properties".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/WalletProvider.tsx` around lines 61 - 62, Wrap the derived values connectedAddress and isConnected in useMemo to avoid re-computation on every render: import useMemo and replace the direct assignments with useMemo hooks that compute connectedAddress from walletAddress and appKitAddress, and isConnected from walletAddress and appKitConnected, each listing their respective dependencies (walletAddress, appKitAddress for connectedAddress; walletAddress, appKitConnected for isConnected).packages/swap-widget/src/types/index.ts (1)
279-302: Unsafeas ChainIdcasts on unvalidatedstringinput.
isEvmChainId,getEvmNetworkId, andgetChainTypeall acceptstringbut cast toChainIdwithout validation. If called with an invalid/malformed string,fromChainIdcould throw at runtime.Consider adding a guard or using a narrower parameter type:
💡 Option: accept ChainId directly
-export const isEvmChainId = (chainId: string): boolean => { - const { chainNamespace } = fromChainId(chainId as ChainId) +export const isEvmChainId = (chainId: ChainId): boolean => { + const { chainNamespace } = fromChainId(chainId) return chainNamespace === CHAIN_NAMESPACE.Evm }Apply the same pattern to
getEvmNetworkIdandgetChainType.As per coding guidelines, "NEVER use type assertions without proper validation in TypeScript" and "ALWAYS use type guards instead of type assertions when possible in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/types/index.ts` around lines 279 - 302, The functions isEvmChainId, getEvmNetworkId, and getChainType cast an arbitrary string to ChainId and call fromChainId which can throw on invalid input; change their signatures to accept ChainId where callers can provide a validated value or add a runtime guard (e.g., implement and use an isValidChainId(type guard) that checks the string against known chain ids) and only call fromChainId when the guard returns true, otherwise return safe defaults (false for isEvmChainId, NaN or throw for getEvmNetworkId, and 'other' for getChainType) or surface a clear error; update all call sites to pass ChainId when possible or handle the guard’s fallback.packages/swap-widget/src/components/InputStep.tsx (2)
50-52:sendprop is destructured as_sendbut never used.Per coding guidelines, unused parameters should be removed from the interface rather than prefixed with underscore. The component delegates all actions through dedicated handler callbacks (
onButtonClick,onSellAmountChange, etc.), sosendis unnecessary.Suggested fix
Remove
sendfromInputStepPropsand the destructuring:type InputStepProps = { context: SwapMachineContext - send: (event: SwapMachineEvent) => void rates: TradeRate[] // ... } export const InputStep = ({ context, - send: _send, rates, // ... }: InputStepProps) => {Also remove the import of
SwapMachineEventfrom'../machines/types'if no longer needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/InputStep.tsx` around lines 50 - 52, Remove the unused `send` prop from the InputStep component and its props type: update InputStepProps to no longer include `send`, and delete `send` from the InputStep destructuring (the component uses handler callbacks like onButtonClick/onSellAmountChange instead). Also remove the now-unused import `SwapMachineEvent` from '../machines/types' if it was only used for the `send` prop type, and run a quick build/type check to ensure no remaining references to `send` or SwapMachineEvent exist.
94-97: Redundant boolean aliases —canExecuteUtxois identical toisSellAssetUtxo,canExecuteSolanatoisSellAssetSolana.These aliases add a layer of indirection without any distinct semantics, making the
buttonTextandisButtonDisabledlogic harder to follow. Conditions likeisSellAssetUtxo && canExecuteUtxo(Line 105) are always tautological.Not blocking, but worth noting for future clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/InputStep.tsx` around lines 94 - 97, Remove the redundant aliases canExecuteUtxo and canExecuteSolana in the InputStep component and use the original booleans isSellAssetUtxo and isSellAssetSolana directly; update any logic that references canExecuteUtxo/canExecuteSolana (including buttonText and isButtonDisabled computations) to use isSellAssetUtxo/isSellAssetSolana, remove tautological checks like isSellAssetUtxo && canExecuteUtxo, and adjust isUnsupportedChain (currently using !isSellAssetEvm && !isSellAssetUtxo && !isSellAssetSolana) as needed; you may keep or rename canExecuteDirectly (currently equal to isSellAssetEvm) if it provides clearer semantics, but ensure all references within InputStep consistently use the chosen symbols.packages/swap-widget/src/demo/App.tsx (1)
155-161:handleSwapSuccessandhandleSwapErrorare not wrapped inuseCallback.These are passed as props to
SwapWidget, which may cause unnecessary re-renders. Since this is demo code, deferring is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/demo/App.tsx` around lines 155 - 161, The two handler functions handleSwapSuccess and handleSwapError should be memoized with React's useCallback to avoid creating new function references on every render when passed to SwapWidget; update the definitions of handleSwapSuccess and handleSwapError to use useCallback(() => { ... }, []) (or include any external deps if needed) and ensure useCallback is imported from React so the props remain stable and prevent unnecessary re-renders.packages/swap-widget/src/components/SwapWidget.tsx (1)
506-593: Significant duplication betweenSwapWidgetWithExternalWalletandSwapWidgetWithInternalWallet.Both components compute identical fallback logic for disabled/allowed chain/asset IDs (lines 534-541 vs 579-586) and pass the same set of props to
SwapWidgetCore. This could be extracted into a shared helper or component, but given the PR scope and the author's preference for focused changes, deferring is reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/components/SwapWidget.tsx` around lines 506 - 593, The two components SwapWidgetWithExternalWallet and SwapWidgetWithInternalWallet duplicate the same prop construction and fallback logic for disabled/allowed chain/asset IDs and the SwapWidgetCore prop list; extract that shared logic into a small helper (e.g., buildSwapCoreProps or getSwapCoreProps) that accepts the incoming props object plus apiClient, walletClient and enableWalletConnection flag and returns the consolidated props object (including sellDisabledAssetIds, buyDisabledAssetIds, sellDisabledChainIds, buyDisabledChainIds, sellAllowedChainIds, buyAllowedChainIds, sellAllowedAssetIds, buyAllowedAssetIds, isBuyAssetLocked, default* values, theme, callbacks, showPoweredBy, defaultSlippage, defaultReceiveAddress); then call <helper>(props, apiClient, walletClient, true|false) from each component and spread its result into SwapWidgetCore, leaving only the differing wrapper specifics (External uses provided walletClient and enableWalletConnection=false; Internal uses InternalWalletProvider to get walletClient and enableWalletConnection=true).packages/swap-widget/src/machines/swapMachine.ts (1)
49-54: Optional: replace manualassetIdstring parsing withfromAssetIdfor ERC-20 detection.The inline
split('/')…split(':')parse is fragile if the CAIP-19 format ever changes.fromAssetIdfrom@shapeshiftoss/caip(already a transitive dep) exposesassetNamespacedirectly.♻️ Proposed refactor
- isApprovalRequired: ({ context, event }) => { - const quote = (event as { type: 'QUOTE_SUCCESS'; quote: QuoteResponse }).quote - if (quote?.approval?.isRequired !== true || context.chainType !== 'evm') return false - const namespace = context.sellAsset.assetId.split('/')[1]?.split(':')[0] - return namespace === 'erc20' - }, + isApprovalRequired: ({ context, event }) => { + const quote = (event as { type: 'QUOTE_SUCCESS'; quote: QuoteResponse }).quote + if (quote?.approval?.isRequired !== true || context.chainType !== 'evm') return false + const { assetNamespace } = fromAssetId(context.sellAsset.assetId) + return assetNamespace === 'erc20' + },Import
fromAssetIdat the top of the file:+import { fromAssetId } from '@shapeshiftoss/caip'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swap-widget/src/machines/swapMachine.ts` around lines 49 - 54, The isApprovalRequired guard currently parses context.sellAsset.assetId with split to detect ERC‑20; replace that fragile parsing by importing fromAssetId from `@shapeshiftoss/caip` and use fromAssetId(context.sellAsset.assetId).assetNamespace to check for 'erc20' (keep the existing checks for quote.approval.isRequired and context.chainType !== 'evm'); update the import list at the top of the file to include fromAssetId and remove the manual split logic in isApprovalRequired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swap-widget/src/components/ApprovalStep.tsx`:
- Around line 12-14: The current explorerUrl computation in ApprovalStep uses a
hardcoded etherscan fallback which can point to the wrong explorer for
non-mainnet EVM chains; update the logic that builds explorerUrl (the expression
using approvalTxHash and sellAsset.explorerTxLink) to derive a chain-appropriate
default explorer when sellAsset.explorerTxLink is missing (for example, look up
a default explorer base by sellAsset.chainId or use a provided chain-to-explorer
map), fall back to undefined only if no suitable explorer can be determined, and
ensure explorerUrl still concatenates the transaction hash when available.
In `@packages/swap-widget/src/components/InputStep.tsx`:
- Around line 125-134: The early return using isSellAssetEvm makes the later
canExecuteDirectly check unreachable because canExecuteDirectly ===
isSellAssetEvm; fix by removing the redundant check or reordering so the
canonical gate is canExecuteDirectly: update the logic in the label resolution
(the function that contains the sequence of ifs) to use canExecuteDirectly
consistently (either replace the initial if (!isSellAssetEvm) with if
(!canExecuteDirectly) and remove the later if (!canExecuteDirectly) or drop the
later dead branch), referencing isSellAssetEvm and canExecuteDirectly to ensure
there are no duplicated/conflicting checks.
In `@packages/swap-widget/src/components/QuotesModal.css`:
- Around line 139-146: The hover state for .ssw-quote-row.ssw-best becomes
visually identical to its base state when consumers set --ssw-accent-light
because both rules read the same variable; update the CSS to use a distinct
token for hover (e.g., reference .ssw-quote-row.ssw-best:hover should use
var(--ssw-accent-light-hover, rgba(...,0.1)) ) or programmatically adjust the
hover color (e.g., apply a filter/alpha change) so the hover is always different
from the base; add the new token (--ssw-accent-light-hover) to the widget's
token declarations so consumers can override it.
In `@packages/swap-widget/src/components/SwapWidget.tsx`:
- Around line 353-354: Change the walletClient prop type in the
SwapWidgetCoreProps interface from unknown to the concrete WalletClient type
used by the project (e.g., import WalletClient from the library you use) so you
don't need unsafe casts; update the SwapWidgetCoreProps declaration and the
component signature to use WalletClient, then update the useMemo that currently
does an `as WalletClient` cast (referencing the useMemo/function that derives
signer/client behavior) to operate directly on the typed walletClient value and
remove the cast. Ensure any other references to walletClient in SwapWidget.tsx
are typed consistently with the new WalletClient type.
In `@packages/swap-widget/src/components/TokenSelectModal.tsx`:
- Around line 260-264: The effect watching visibleRangeAssetIds currently
triggers refetchSpecific on every scroll tick; debounce the updates so
refetchSpecific is called fewer times by creating a debounced version of
visibleRangeAssetIds (e.g., inside the useEffect for visibleRangeAssetIds or via
a small custom hook) that delays applying the latest array for ~150-300ms and
cancels/clears the timeout on cleanup; update the effect to call refetchSpecific
only when the debounced value changes and keep references to refetchSpecific
stable (or include it in deps) so functions like useEffect,
visibleRangeAssetIds, and refetchSpecific are the only symbols you modify.
In `@packages/swap-widget/src/machines/swapMachine.ts`:
- Line 58: The isExecuteError guard function is dead code and should be removed:
delete the isExecuteError entry that checks context.errorSource ===
'EXECUTE_ERROR' from the machine's guards configuration (where guards are
declared) so the guard map no longer contains this unused symbol; ensure no
transitions reference isExecuteError (the error state's RETRY list already uses
canRetry for EXECUTE_ERROR handling), and run tests/lint to confirm no remaining
references to isExecuteError.
- Around line 157-171: The resetSwapState action uses XState v5's assign which
merges the returned object into context, so omitted keys (like selectedRate)
remain stale; update the resetSwapState assign (in swapMachine) to explicitly
reset selectedRate to null (or its initial value) so completed-swap TradeRate
doesn't persist after RESET and downstream hooks won't consume a stale rate;
locate resetSwapState in swapMachine.ts and add selectedRate: null to the
returned object (and optionally reset any other transient rate-related fields).
---
Outside diff comments:
In `@packages/swap-widget/src/types/index.ts`:
- Around line 284-287: getEvmNetworkId currently does parseInt(chainReference,
10) which yields NaN for non-numeric chain references; update getEvmNetworkId to
validate chainReference (from fromChainId(chainId as ChainId)) before
parsing—e.g., ensure it matches a numeric pattern or
Number.isFinite(Number(chainReference))—and then either return the parsed
integer or throw a descriptive Error (or return a documented fallback) so
callers won't receive NaN when switching networks or calling RPCs.
---
Duplicate comments:
In `@packages/swap-widget/src/components/SwapWidget.css`:
- Around line 15-17: The CSS block in SwapWidget.css currently forces a rigid
420px layout with "width: 100%; min-width: 420px; max-width: 420px" which breaks
on narrow viewports; remove or change the "min-width: 420px" declaration inside
the SwapWidget.css width block (the same declarations shown) so the widget can
shrink on mobile—leave "width: 100%" and "max-width: 420px" (or set a responsive
min-width like min-width: 0) to prevent horizontal overflow and ensure the
component adapts to viewports under 420px.
In `@packages/swap-widget/src/components/TokenSelectModal.tsx`:
- Around line 211-252: The useMemo for sortedAssets currently performs side
effects by mutating prevFilteredRef, balanceSortDoneRef, and sortedAssetsRef;
make sortedAssets a pure computation (no ref writes) and move the ref updates
into a separate useEffect that reacts to changes in filteredAssets, balances,
and marketData. Specifically, have useMemo only compute and return the sorted
array (reference sortedAssetsMemo), and add a useEffect that compares
prevFilteredRef.current to filteredAssets, sets balanceSortDoneRef.current and
sortedAssetsRef.current as needed, and updates
prevFilteredRef.current—referencing the sortedAssets variable created by useMemo
and the existing refs (prevFilteredRef, balanceSortDoneRef, sortedAssetsRef) and
functions (bnOrZero) to locate where to change behavior.
In `@packages/swap-widget/src/hooks/useBitcoinSigning.ts`:
- Around line 11-15: The SignInput type is used in the public SignPsbtParams
signature but is not exported; update the declaration of SignInput to be
exported (export type SignInput = { address: string; index: number;
sighashTypes?: number[] }) so callers can import it, and verify that
SignPsbtParams still references SignInput[] for its signInputs field; ensure the
exported name matches any places referencing SignInput.
---
Nitpick comments:
In `@packages/swap-widget/src/components/ExecutionStep.tsx`:
- Line 8: The ExecutionStep component currently destructures unused props as
_context and _send from ExecutionStepProps; either stop destructuring them and
accept a single unused prop (e.g., _props: ExecutionStepProps or _:
ExecutionStepProps) to show intent, or narrow the component signature to only
the required props by removing context and send from the parameter type; update
the ExecutionStep declaration (symbol ExecutionStep) to one of these two options
and remove the unused identifiers _context and _send accordingly so the
signature matches actual usage or the shared interface requirements.
In `@packages/swap-widget/src/components/InputStep.tsx`:
- Around line 50-52: Remove the unused `send` prop from the InputStep component
and its props type: update InputStepProps to no longer include `send`, and
delete `send` from the InputStep destructuring (the component uses handler
callbacks like onButtonClick/onSellAmountChange instead). Also remove the
now-unused import `SwapMachineEvent` from '../machines/types' if it was only
used for the `send` prop type, and run a quick build/type check to ensure no
remaining references to `send` or SwapMachineEvent exist.
- Around line 94-97: Remove the redundant aliases canExecuteUtxo and
canExecuteSolana in the InputStep component and use the original booleans
isSellAssetUtxo and isSellAssetSolana directly; update any logic that references
canExecuteUtxo/canExecuteSolana (including buttonText and isButtonDisabled
computations) to use isSellAssetUtxo/isSellAssetSolana, remove tautological
checks like isSellAssetUtxo && canExecuteUtxo, and adjust isUnsupportedChain
(currently using !isSellAssetEvm && !isSellAssetUtxo && !isSellAssetSolana) as
needed; you may keep or rename canExecuteDirectly (currently equal to
isSellAssetEvm) if it provides clearer semantics, but ensure all references
within InputStep consistently use the chosen symbols.
In `@packages/swap-widget/src/components/SwapWidget.tsx`:
- Around line 506-593: The two components SwapWidgetWithExternalWallet and
SwapWidgetWithInternalWallet duplicate the same prop construction and fallback
logic for disabled/allowed chain/asset IDs and the SwapWidgetCore prop list;
extract that shared logic into a small helper (e.g., buildSwapCoreProps or
getSwapCoreProps) that accepts the incoming props object plus apiClient,
walletClient and enableWalletConnection flag and returns the consolidated props
object (including sellDisabledAssetIds, buyDisabledAssetIds,
sellDisabledChainIds, buyDisabledChainIds, sellAllowedChainIds,
buyAllowedChainIds, sellAllowedAssetIds, buyAllowedAssetIds, isBuyAssetLocked,
default* values, theme, callbacks, showPoweredBy, defaultSlippage,
defaultReceiveAddress); then call <helper>(props, apiClient, walletClient,
true|false) from each component and spread its result into SwapWidgetCore,
leaving only the differing wrapper specifics (External uses provided
walletClient and enableWalletConnection=false; Internal uses
InternalWalletProvider to get walletClient and enableWalletConnection=true).
In `@packages/swap-widget/src/components/TokenSelectModal.tsx`:
- Around line 95-104: Convert allowedAssetIds into a Set once and use O(1)
lookups in both the allowedAssetChainIds and filteredAssets computations: create
a memoized allowedAssetIdSet (using useMemo) derived from allowedAssetIds,
update the allowedAssetChainIds logic to call
allowedAssetIdSet.has(asset.assetId) instead of allowedAssetIds.includes(...),
and similarly replace includes with .has in the filteredAssets calculation;
ensure the new memo depends on allowedAssetIds and then use that Set in both
allowedAssetChainIds and filteredAssets to avoid repeated O(n×m) scans.
- Around line 456-468: The fiat value calculation is duplicated: the BigNumber
math done in sortedAssets (referenced as sortedAssets) is recomputed in the
render block using
bnOrZero(balance.balance).div(...).times(marketData[asset.assetId].price); fix
by computing fiat values once when building sortedAssets (or immediately after)
and store them in a lookup (e.g., Map<assetId, number> or an object keyed by
asset.assetId), then replace the inline BigNumber expression in the
TokenSelectModal render with a simple lookup (using marketData and the
precomputed map) to avoid repeating heavy bnOrZero math on every visible row.
- Around line 417-478: The inline itemContent render function in
TokenSelectModal is recreated every render and causes unnecessary row
re-renders; extract it into a separate memoized component (e.g., TokenRow) that
accepts props needed for rendering (asset, balance, marketData[asset.assetId],
chainInfoMap.get(asset.chainId), loadingAssetIds.has(asset.assetId),
walletAddress/bitcoinAddress/solanaAddress, handleAssetSelect, bnOrZero) and
wrap that component with React.memo (and useCallback for any handlers like
onClick pointing to handleAssetSelect) so rows only re-render when their
specific props change; replace the inline itemContent with a stable function
that returns <TokenRow ... /> to improve scroll performance.
In `@packages/swap-widget/src/components/WalletProvider.tsx`:
- Around line 61-62: Wrap the derived values connectedAddress and isConnected in
useMemo to avoid re-computation on every render: import useMemo and replace the
direct assignments with useMemo hooks that compute connectedAddress from
walletAddress and appKitAddress, and isConnected from walletAddress and
appKitConnected, each listing their respective dependencies (walletAddress,
appKitAddress for connectedAddress; walletAddress, appKitConnected for
isConnected).
In `@packages/swap-widget/src/demo/App.tsx`:
- Around line 155-161: The two handler functions handleSwapSuccess and
handleSwapError should be memoized with React's useCallback to avoid creating
new function references on every render when passed to SwapWidget; update the
definitions of handleSwapSuccess and handleSwapError to use useCallback(() => {
... }, []) (or include any external deps if needed) and ensure useCallback is
imported from React so the props remain stable and prevent unnecessary
re-renders.
In `@packages/swap-widget/src/machines/guards.ts`:
- Line 21: Extract the magic number 3 into a named constant (e.g.,
MAX_RETRY_COUNT) and update the guard to use it: change the comparison in
canRetry (which accepts SwapMachineContext) to reference MAX_RETRY_COUNT so the
retry limit is centralized, follows UPPER_SNAKE_CASE, and is easier to adjust
and document.
In `@packages/swap-widget/src/machines/swapMachine.ts`:
- Around line 49-54: The isApprovalRequired guard currently parses
context.sellAsset.assetId with split to detect ERC‑20; replace that fragile
parsing by importing fromAssetId from `@shapeshiftoss/caip` and use
fromAssetId(context.sellAsset.assetId).assetNamespace to check for 'erc20' (keep
the existing checks for quote.approval.isRequired and context.chainType !==
'evm'); update the import list at the top of the file to include fromAssetId and
remove the manual split logic in isApprovalRequired.
In `@packages/swap-widget/src/types/index.ts`:
- Around line 279-302: The functions isEvmChainId, getEvmNetworkId, and
getChainType cast an arbitrary string to ChainId and call fromChainId which can
throw on invalid input; change their signatures to accept ChainId where callers
can provide a validated value or add a runtime guard (e.g., implement and use an
isValidChainId(type guard) that checks the string against known chain ids) and
only call fromChainId when the guard returns true, otherwise return safe
defaults (false for isEvmChainId, NaN or throw for getEvmNetworkId, and 'other'
for getChainType) or surface a clear error; update all call sites to pass
ChainId when possible or handle the guard’s fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
packages/public-api/src/docs/openapi.tspackages/public-api/src/routes/quote.tspackages/swap-widget/package.jsonpackages/swap-widget/src/components/AddressInputModal.csspackages/swap-widget/src/components/ApprovalStep.tsxpackages/swap-widget/src/components/ExecutionStep.tsxpackages/swap-widget/src/components/InputStep.tsxpackages/swap-widget/src/components/QuoteSelector.csspackages/swap-widget/src/components/QuotesModal.csspackages/swap-widget/src/components/SettingsModal.csspackages/swap-widget/src/components/StatusStep.tsxpackages/swap-widget/src/components/SwapWidget.csspackages/swap-widget/src/components/SwapWidget.tsxpackages/swap-widget/src/components/TokenSelectModal.csspackages/swap-widget/src/components/TokenSelectModal.tsxpackages/swap-widget/src/components/WalletProvider.tsxpackages/swap-widget/src/constants/defaults.tspackages/swap-widget/src/constants/viemChains.tspackages/swap-widget/src/contexts/SwapWalletContext.tsxpackages/swap-widget/src/demo/App.csspackages/swap-widget/src/demo/App.tsxpackages/swap-widget/src/hooks/useBalances.tspackages/swap-widget/src/hooks/useBitcoinSigning.tspackages/swap-widget/src/hooks/useStatusPolling.tspackages/swap-widget/src/hooks/useSwapApproval.tspackages/swap-widget/src/hooks/useSwapDisplayValues.tspackages/swap-widget/src/hooks/useSwapExecution.tspackages/swap-widget/src/hooks/useSwapHandlers.tspackages/swap-widget/src/hooks/useSwapQuoting.tspackages/swap-widget/src/machines/SwapMachineContext.tspackages/swap-widget/src/machines/__tests__/guards.test.tspackages/swap-widget/src/machines/__tests__/setup.test.tspackages/swap-widget/src/machines/__tests__/swapMachine.test.tspackages/swap-widget/src/machines/__tests__/types.test.tspackages/swap-widget/src/machines/guards.tspackages/swap-widget/src/machines/swapMachine.tspackages/swap-widget/src/machines/types.tspackages/swap-widget/src/types/index.tspackages/swap-widget/vite.config.tspackages/swap-widget/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/swap-widget/src/machines/types.ts
- packages/public-api/src/docs/openapi.ts
- packages/swap-widget/src/hooks/useSwapQuoting.ts
- packages/swap-widget/src/hooks/useSwapExecution.ts
- packages/swap-widget/vite.config.ts
- packages/swap-widget/src/components/TokenSelectModal.css
- packages/swap-widget/src/machines/tests/types.test.ts
- packages/swap-widget/src/constants/viemChains.ts
- packages/swap-widget/src/components/StatusStep.tsx
- packages/swap-widget/src/hooks/useBalances.ts
- packages/swap-widget/src/contexts/SwapWalletContext.tsx
- packages/swap-widget/src/machines/SwapMachineContext.ts
- packages/swap-widget/src/components/AddressInputModal.css
- packages/public-api/src/routes/quote.ts
- packages/swap-widget/src/components/SettingsModal.css
- packages/swap-widget/package.json
- packages/swap-widget/src/hooks/useStatusPolling.ts
- packages/swap-widget/src/hooks/useSwapApproval.ts
- packages/swap-widget/src/hooks/useSwapHandlers.ts
- packages/swap-widget/vitest.config.ts
NeOMakinG
dismissed
0xApotheosis’s stale review
Dismissing to avoid getting blocked!
3 participants
Description
Major cleanup and bug fixes for the swap widget to make multi-chain swaps (EVM, Bitcoin, Solana) functional through Reown AppKit wallets.
Swap Widget — Code Cleanup
actors.ts(142 lines) and duplicatechainUtils.tsSwapStepenum,SWAP_TOKENSandEXECUTEevent typesswapMachine.tsnow importsDEFAULT_SELL_ASSET/DEFAULT_BUY_ASSETfromconstants/defaults.tsuseSwapApproval.tsuses viem'serc20Abiinstead of a hand-rolled ERC-20 ABISwap Widget — Execution Fixes
TransactionDatadiscriminated union (txData.type !== 'solana') instead of accessing a non-existentsolanaTransactionMetadatafieldutxo_psbtvsutxo_deposit) with proper type narrowing. Includes asendTransferfallback when PSBT signing fails (non-user-rejection errors)Swap Widget — Bitcoin Wallet Fixes
useState+useEffect+getAccountAddresses()withuseMemodriven byuseAppKitAccount({ namespace: 'bip122' }). Address now reactively updates when user switches accounts in ReownappKitBtcAddress(the user's active selection in Reown) directly, instead of overriding with our own address-type preferencesendTransferparam name: AppKit'sBitcoinConnectorexpects{ recipient, amount }— we were passing{ recipientAddress, amount }, causing Leather to receive an empty address and fail with validation errorsendTransfertype override fromBitcoinConnectorExtended, using the inheritedBitcoinConnector.sendTransferinterface insteadSwap Widget — UI
Public API — UTXO Transaction Data
utxo_depositinstead ofutxo_psbt: PSBT signing through AppKit is fundamentally broken (Leather'ssignAtIndexonly reads the first input, Phantom doesn't supportsignPSBTat all). Relay provides unique deposit addresses per swap intent, sosendTransferto the deposit address works without needing OP_RETURNdepositAddressandvaluefields toUtxoPsbtTransactionData: Fallback data for clients that still receive PSBT type from other sourcesTypes Package
UtxoPsbtTransactionDatawith optionaldepositAddressandvaluefieldsRisk
Medium — changes EVM/UTXO/Solana execution paths in the swap widget and the UTXO transaction data format in the public API.
Testing
Engineering
sendTransactionsendTransferto Relay deposit address (not PSBT)POST /quotewith BTC sell asset + Relay swapper → response should havetransactionData.type === 'utxo_deposit'(notutxo_psbt)Operations
Test by connecting various wallets (Leather, Phantom) through the swap widget's built-in AppKit connection and executing swaps on EVM, Bitcoin, and Solana chains.
Screenshots (if applicable)
https://mempool.space/tx/ae951d55a275fa3e92d241338a92ef9c61ff3f7084d3f7c6bffafae2886d3eea
https://jam.dev/c/b4c16e0f-77de-4363-a49f-50c553686244
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests