feat: rework our account management source of truth to make it more robust #144
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors wallet connection handling to use AppKit's modal provider directly instead of wagmi hooks, adds wallet address validation to transaction and swap execution flows, improves network switch concurrency control, and optimizes wallet state management in the provider. A model configuration string is also updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agentic-chat/src/hooks/useSendExecution.ts (1)
158-160: Persisted send state can miss completed steps due to stalestatesnapshotThe success path constructs
finalCompletedStepsat line 205 fromstate.completedSteps, which is stale and doesn't include steps added by the priorsetStatecalls (lines 170–183). Steps likeSendStep.NETWORK_SWITCHwill be missing from the persisted state, causing them to display asSKIPPEDafter reload.Fix by capturing the final state within the
setStatecallback usingcurrent(draft), mirroring the error handling pattern at lines 242–246. Replace lines 204–228 to capturefinalStatefrom the lastsetStatemutation and use that for persistence instead of reconstructing from the outerstate.
🧹 Nitpick comments (3)
apps/agentic-chat/src/hooks/useApprovedChains.ts (1)
11-27: Universal provider session parsing for approved chains looks solidThe new
getApprovedChainsFromProvider+useApprovedChainsflow correctly guards onsession?.namespaces, pulls botheip155andsolanaaccounts, and usesisConnectedto driveenabledand cache keys so data refreshes cleanly across connect/disconnect cycles. If you later support additional namespaces, you may want to extendWalletConnectSessionand the extractor rather than silently ignoring them.Also applies to: 30-44
apps/agentic-chat/src/hooks/useAutoNetworkSwitch.ts (1)
4-5: Concurrency guard around auto network switching looks correctUsing
switchInProgressReftogether with theisModalOpencheck (Lines 19–21, 35–43) should effectively prevent overlappingmodal.switchNetworkcalls and the race conditions they can cause when connection state flaps between EVM and Solana; the 500ms cooldown is a reasonable lightweight backoff.Also applies to: 11-12, 18-44
apps/agentic-chat/src/utils/walletErrors.ts (1)
1-20:verifyWalletForTransactioncentralizes wallet invariants cleanlyThe helper correctly derives the active address from
getWalletState()and enforces both “wallet connected” and “from address unchanged” invariants with clear error messages, which should keep send/swap behavior consistent. Note that Line 12 treats any non‑EVMchainNamespaceas Solana; that matches current EVM/Solana support, but if you later add other namespaces you’ll likely want to extend this switch instead of defaulting tosolanaAddress, and consider gating calls on a known‑connected state to avoid rare false “Wallet disconnected” errors immediately after connect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/agentic-chat/src/components/Portfolio/PortfolioDrawer.tsx(2 hunks)apps/agentic-chat/src/hooks/useApprovedChains.ts(1 hunks)apps/agentic-chat/src/hooks/useAutoNetworkSwitch.ts(3 hunks)apps/agentic-chat/src/hooks/useSendExecution.ts(3 hunks)apps/agentic-chat/src/hooks/useSwapExecution.tsx(2 hunks)apps/agentic-chat/src/hooks/useWalletConnection.ts(1 hunks)apps/agentic-chat/src/providers/ChatProvider.tsx(3 hunks)apps/agentic-chat/src/utils/walletErrors.ts(1 hunks)apps/agentic-server/src/models.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/agentic-chat/src/utils/walletErrors.ts (2)
apps/agentic-chat/src/hooks/useWalletConnection.ts (1)
getWalletState(20-22)packages/caip/src/constants.ts (1)
CHAIN_NAMESPACE(46-54)
apps/agentic-chat/src/providers/ChatProvider.tsx (1)
apps/agentic-chat/src/hooks/useWalletConnection.ts (2)
useWalletConnection(24-44)getWalletState(20-22)
apps/agentic-chat/src/hooks/useSendExecution.ts (1)
apps/agentic-chat/src/utils/walletErrors.ts (1)
verifyWalletForTransaction(10-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: main
🔇 Additional comments (6)
apps/agentic-server/src/models.ts (1)
20-28: Confirm Venice model identifier forclaude-opus-45Line 26: the config change itself is fine; just double‑check that
'claude-opus-45'is the exact model ID Venice expects in your environment (or consider wiring it via an env var if you expect to switch models frequently).apps/agentic-chat/src/hooks/useSendExecution.ts (1)
161-168: Pre-send wallet verification aligns send with swap flowCalling
verifyWalletForTransaction(chainNamespace, tx.from)before markingPREPARATIONcomplete ensures sends fail fast if the wallet disconnects or thefromaddress changes between planning and execution, and keeps this flow consistent with the swap pre-check.apps/agentic-chat/src/components/Portfolio/PortfolioDrawer.tsx (1)
44-58: Cache invalidation on disconnect helps avoid stale portfolio/chain dataInvalidating the
['portfolio']and['approvedChains']queries inhandleDisconnect(Lines 47–48) before actually disconnecting should ensure portfolio and approved-chain data are refetched cleanly on the next connection instead of showing stale results tied to the previous wallet.apps/agentic-chat/src/hooks/useSwapExecution.tsx (1)
185-193: Swap flow now validates wallet before proceeding
verifyWalletForTransaction(chainNamespace, swapTx.from)(Line 191) adds the same runtime wallet check used in the send flow, so swaps will now fail early on disconnects orfrom-address changes between quote and execution rather than getting as far as network switch or on-chain calls.apps/agentic-chat/src/providers/ChatProvider.tsx (1)
7-8: Centralized wallet state wiring for chat transport looks goodRunning
useWalletConnection()for its side effects (Lines 46–48) and havingDefaultChatTransport’sbodycallback pull{ evmAddress, solanaAddress, approvedChainIds }fromgetWalletState()(Lines 61–69) gives you always‑current wallet data in the chat payload without juggling refs; the empty dependency array onuseMemois appropriate since the body reads live module state, not closed-over values.Also applies to: 45-48, 61-71
apps/agentic-chat/src/hooks/useWalletConnection.ts (1)
25-25: Verify the migration from wagmi to useAppKitAccount.The migration from wagmi's account hook to
useAppKitAccount({ namespace: 'eip155' })appears correct for EVM address retrieval, but please verify:
- The AppKit hook provides the same guarantees as wagmi (address format, normalization, reactivity)
- The
namespace: 'eip155'correctly maps to EVM chains- Downstream code expecting wagmi's behavior won't break
Reown AppKit useAppKitAccount hook documentation and EIP155 namespace
premiumjibles
force-pushed
the
account-robustness
branch
from
686531d to
538862f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/agentic-chat/src/hooks/useApprovedChains.ts (1)
1-26: Tighten session/namespace narrowing for TypeScript friendlinessThe runtime guard on
session?.namespacesis sound, but TypeScript may still complain aboutsessionpossibly beingundefinedwhen you later accesssession.namespaces(depending on compiler options). You can improve both readability and type‑narrowing by hoistingnamespacesinto a local:async function getApprovedChainsFromProvider(): Promise<string[]> { const provider = await modal?.getUniversalProvider() const session = provider?.session as WalletConnectSession | undefined - if (!session?.namespaces) { - return [] - } - - return [ - ...extractChainIdsFromAccounts(session.namespaces.eip155?.accounts ?? []), - ...extractChainIdsFromAccounts(session.namespaces.solana?.accounts ?? []), - ] + const namespaces = session?.namespaces + if (!namespaces) { + return [] + } + + return [ + ...extractChainIdsFromAccounts(namespaces.eip155?.accounts ?? []), + ...extractChainIdsFromAccounts(namespaces.solana?.accounts ?? []), + ] }Same behavior, but TS should now be happier and the control flow is clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/agentic-chat/src/components/Portfolio/PortfolioDrawer.tsx(2 hunks)apps/agentic-chat/src/hooks/useApprovedChains.ts(1 hunks)apps/agentic-chat/src/hooks/useAutoNetworkSwitch.ts(3 hunks)apps/agentic-chat/src/hooks/useSendExecution.ts(4 hunks)apps/agentic-chat/src/hooks/useSwapExecution.tsx(3 hunks)apps/agentic-chat/src/hooks/useWalletConnection.ts(1 hunks)apps/agentic-chat/src/providers/ChatProvider.tsx(2 hunks)apps/agentic-server/src/models.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/agentic-server/src/models.ts
- apps/agentic-chat/src/hooks/useSendExecution.ts
- apps/agentic-chat/src/components/Portfolio/PortfolioDrawer.tsx
- apps/agentic-chat/src/hooks/useAutoNetworkSwitch.ts
- apps/agentic-chat/src/hooks/useWalletConnection.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (2)
apps/agentic-chat/src/hooks/useWalletConnection.ts (1)
useWalletConnection(5-18)packages/caip/src/constants.ts (1)
CHAIN_NAMESPACE(46-54)
apps/agentic-chat/src/providers/ChatProvider.tsx (1)
apps/agentic-chat/src/hooks/useWalletConnection.ts (1)
useWalletConnection(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: main
🔇 Additional comments (4)
apps/agentic-chat/src/providers/ChatProvider.tsx (2)
46-48: LGTM! Cleaner wallet state management.This refactoring consolidates three separate refs into a single
walletRef, eliminating the need for a separateuseEffectto keep them synchronized. The patternwalletRef.current = walleton every render ensures the transport body always accesses the latest wallet state, while maintaining consistency across all three properties (evmAddress, solanaAddress, approvedChainIds).
67-69: LGTM! Correct usage of ref pattern in transport body.The body function correctly reads from
walletRef.currentto ensure each transport request uses the current wallet state at dispatch time. Since the transport is memoized with an empty dependency array (line 72), this ref pattern is the appropriate way to access the latest wallet values without recreating the transport on every wallet change.apps/agentic-chat/src/hooks/useApprovedChains.ts (1)
29-44: Hook behavior and query configuration look solidUsing
useAppKitAccountper namespace and OR‑ingisConnectedto gate the query, plus returningdata ?? [], keeps the public API consistentlystring[]and avoids leaking stale chains when disconnected. The query options (enabled,staleTime,gcTime) are coherent with that behavior; no issues from my side here.apps/agentic-chat/src/hooks/useSwapExecution.tsx (1)
21-21: Hook wiring touseWalletConnectionlooks correctImporting and destructuring
evmAddress/solanaAddressfromuseWalletConnectioninsideuseSwapExecutionis consistent with the new wallet source‑of‑truth pattern and keeps this hook aligned with the rest of the app.Also applies to: 162-162
| const currentAddress = chainNamespace === CHAIN_NAMESPACE.Evm ? evmAddress : solanaAddress | ||
| if (!currentAddress) { | ||
| throw new Error('Wallet disconnected. Please reconnect and try again.') | ||
| } | ||
| if (currentAddress.toLowerCase() !== swapTx.from.toLowerCase()) { | ||
| throw new Error('Wallet address changed. Please re-initiate the swap.') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/agentic-chat/src/hooks/useSwapExecution.tsx | sed -n '180,210p'Repository: shapeshift/agentic-chat
Length of output: 1481
🏁 Script executed:
git ls-files | grep -E "(enum|const).*[Cc]hain" | head -20Repository: shapeshift/agentic-chat
Length of output: 49
🏁 Script executed:
rg "CHAIN_NAMESPACE" --type ts --type tsx -A 2 -B 2 | head -60Repository: shapeshift/agentic-chat
Length of output: 94
🏁 Script executed:
rg "solanaAddress|evmAddress" apps/agentic-chat/src/hooks/useSwapExecution.tsx -B 3 -A 3Repository: shapeshift/agentic-chat
Length of output: 753
🏁 Script executed:
rg "CHAIN_NAMESPACE\s*[=:]" -B 2 -A 10 --type ts | head -100Repository: shapeshift/agentic-chat
Length of output: 693
🏁 Script executed:
rg "enum.*CHAIN_NAMESPACE|const.*CHAIN_NAMESPACE" -B 2 -A 15 --type tsRepository: shapeshift/agentic-chat
Length of output: 4245
🏁 Script executed:
rg "fromChainId" -B 2 -A 5 --type ts | head -80Repository: shapeshift/agentic-chat
Length of output: 5960
🏁 Script executed:
rg "swapTx.*from|swapTx.*:" -B 3 -A 3 --type ts | head -60Repository: shapeshift/agentic-chat
Length of output: 4329
🏁 Script executed:
rg "type TransactionData|interface TransactionData" -B 2 -A 10 --type ts --type tsxRepository: shapeshift/agentic-chat
Length of output: 94
🏁 Script executed:
rg "quoteSwap|getRelayRate" apps/agentic-server/src -B 3 -A 8 --type ts | head -100Repository: shapeshift/agentic-chat
Length of output: 4678
🏁 Script executed:
rg "\.from" apps/agentic-server/src/utils/getRelayRate -B 2 -A 2 --type tsRepository: shapeshift/agentic-chat
Length of output: 49
🏁 Script executed:
rg "type TransactionData|interface TransactionData" -A 15Repository: shapeshift/agentic-chat
Length of output: 4025
🏁 Script executed:
rg "createTransaction" apps/agentic-server/src -B 2 -A 10 | head -80Repository: shapeshift/agentic-chat
Length of output: 5490
🏁 Script executed:
rg "txData\[|txData\.from" apps/agentic-server/src -B 3 -A 3Repository: shapeshift/agentic-chat
Length of output: 49
🏁 Script executed:
cat -n apps/agentic-server/src/utils/transactionHelpers.ts | sed -n '1,30p'Repository: shapeshift/agentic-chat
Length of output: 1143
🏁 Script executed:
rg "approvalTarget|txData" apps/agentic-server/src/utils/getRelayRate/getRelayRate.ts -B 5 -A 5Repository: shapeshift/agentic-chat
Length of output: 1613
🏁 Script executed:
rg "from:" apps/agentic-server/src/utils/getRelayRate -B 5 -A 5Repository: shapeshift/agentic-chat
Length of output: 2110
🏁 Script executed:
rg "useWalletConnection" apps/agentic-chat/src/hooks/useSwapExecution.tsx -B 3 -A 8Repository: shapeshift/agentic-chat
Length of output: 891
🏁 Script executed:
rg "evmAddress|solanaAddress" apps/agentic-chat/src/hooks -B 2 -A 2 | head -40Repository: shapeshift/agentic-chat
Length of output: 3142
Tighten address mapping for non-EVM chains and avoid lowercasing Solana addresses
The validation is good, but two issues need fixing:
-
Non-EVM chains incorrectly mapped to
solanaAddress
Currently, any non-EVM namespace (Utxo, CosmosSdk, Sui, Tron, Cardano) usessolanaAddress. If support for these chains is added to swaps, the validation will fail with "Wallet disconnected" or "Wallet address changed" because those chains don't use Solana wallets. Instead:- Explicitly handle only
EvmandSolana - For other namespaces, either skip this check or handle them via dedicated wallet sources
let currentAddress: string | undefined if (chainNamespace === CHAIN_NAMESPACE.Evm) currentAddress = evmAddress else if (chainNamespace === CHAIN_NAMESPACE.Solana) currentAddress = solanaAddress
- Explicitly handle only
-
Lowercasing weakens Solana address validation
if (currentAddress.toLowerCase() !== swapTx.from.toLowerCase()) { ... }
This is correct for EVM hex addresses (case-insensitive), but Solana base58 addresses are case-sensitive. Lowercasing both sides weakens the check for Solana. Use chain-aware normalization instead:
const normalizeAddress = (addr: string) => chainNamespace === CHAIN_NAMESPACE.Evm ? addr.toLowerCase() : addr if (normalizeAddress(currentAddress) !== normalizeAddress(swapTx.from)) { throw new Error('Wallet address changed. Please re-initiate the swap.') }
The same pattern exists in useSendExecution.ts and should be fixed there as well.
🤖 Prompt for AI Agents
In apps/agentic-chat/src/hooks/useSwapExecution.tsx around lines 192 to 198, the
code currently maps any non-EVM namespace to solanaAddress and always lowercases
addresses; update it to only select evmAddress when chainNamespace ===
CHAIN_NAMESPACE.Evm and solanaAddress when chainNamespace ===
CHAIN_NAMESPACE.Solana (for other namespaces skip this check or wire up the
correct wallet source later), and replace unconditional toLowerCase comparisons
with a chain-aware normalizer that lowercases only for EVM addresses (leave
Solana/base58 as-is) before comparing; apply the same exact mapping and
normalization fix to the analogous code in useSendExecution.ts.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.