fix(trade): prevent stale deposit balance reuse after wallet switches - #2430
fix(trade): prevent stale deposit balance reuse after wallet switches#2430Bayyan16 wants to merge 2 commits into
Conversation
|
@Bayyan16 is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1db0a2105b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Independent verification — not an approval (QA/Security own that), just evidence for whoever reviews. Verdict: the tests genuinely bind to the fix. Including a correction to my own first attempt, because it nearly produced a false accusation and the distinction is worth recording. Attempt 1 — unfaithful, and I almost reported it as a defectI removed only the render-time scope check: : walletBalanceSnapshot?.scopeKey === walletBalanceScopeKey → : walletBalanceSnapshotAll 3 tests still passed. On its own that reads as "the tests don't catch the bug". It isn't. This PR has two mechanisms, and I'd only disabled one:
With (1) still live, the stale balance is cleared regardless, so the tests correctly stayed green. My mutation never reproduced #2429. Attempt 2 — faithfulNeutralised both guards, so a foreign-scope snapshot actually survives a wallet switch: That's the load-bearing assertion for the reported bug, and it fails exactly when the bug is present. The other two — "updates to wallet B balance after the replacement request resolves" and "clears a wallet A Max amount and blocks deposit while wallet B balance is unverified" — stay green under mutation, which is right; they cover the recovery path rather than the leak. Two observations1. This is tested at the component layer. That's the standard worth holding: the guard is verified where a user actually experiences it, so it survives a refactor that moves the internals. Contrast #2437 and #2442, where the coverage sits below the layer the bug lives at. 2. The render-time check is redundant for this scenario. The effect's invalidation is what the test binds to. That's fine as belt-and-braces on a balance that gates spending, but worth knowing: if someone later deletes :87/:102 as dead weight, these tests stay green. No changes requested from me. |
|
Thank you for the careful independent verification and for documenting the correction to the first mutation attempt. Understood: the component-level regression genuinely binds to #2429, the effect-level invalidation is the load-bearing protection for this scenario, and the render-time scope check remains defense-in-depth. Since no changes are requested, I’ll leave the implementation and regression coverage unchanged on the current head and await the formal QA/Security review. |
Summary
Fixes #2429.
This PR makes the wallet-balance lifecycle in
DepositWithdrawCardexplicitly scoped to the currently active wallet and collateral mint.Previously, the component stored asynchronously fetched token-balance state without binding that state to the wallet and mint that produced it. During a wallet switch, the previous wallet's balance and balance-derived actions could remain visible or actionable while the replacement wallet's balance request was still pending.
The original patch introduced a wallet-scoped balance snapshot and prevented obsolete asynchronous responses from being consumed under a different wallet scope.
This PR now also closes the remaining existing-account deposit path identified during review:
Maxbutton is cleared at the same time;Together, these protections ensure that neither displayed balance state nor a previously selected deposit amount can cross wallet or collateral-mint boundaries.
Problem
DepositWithdrawCardasynchronously fetches the connected wallet's collateral-token balance through:Before this change, the result was stored independently in local component state:
Those values did not carry any ownership identity proving which wallet and collateral mint produced them.
The component also keeps two representations of the selected transaction amount:
amountis the human-readable value displayed in the input, whilemaxRawRef.currentpreserves the exact native-unitbigintselected through theMaxbutton.Neither value was previously invalidated when the active wallet scope changed.
Stale balance race
The original balance-state race was:
An effect cancellation flag prevented some writes after cleanup, but it did not provide a durable ownership relationship between the stored balance and the wallet for which that balance was fetched.
Stale Max deposit race
An additional existing-account path remained after the initial fix:
Wallet A is active and has an existing trading account.
Wallet A has
900 USDC.The user selects
Maxon the Deposit tab.The component stores:
The user switches to wallet B.
Wallet B's balance request remains pending.
The wallet-scoped balance becomes unavailable:
The over-deposit check cannot compare the requested amount against wallet B because its balance has not resolved.
Without additional protection,
handleSubmitcan still prefer the stale exact value:A deposit amount selected for wallet A may therefore remain actionable after wallet B becomes active.
The transaction would still require wallet B's signature and normal on-chain validation, but the client-side transaction intent and amount would incorrectly originate from wallet A's UI lifecycle.
Security and correctness invariant
Any balance or balance-derived value rendered or consumed by
DepositWithdrawCardmust belong to the exact active identity tuple:After either part of that tuple changes:
Maxvalue must be invalidated;Root cause
The previous implementation had three related lifecycle gaps.
1. Balance data had no provenance
Balance and decimals were stored separately from the wallet and collateral mint that produced them:
A render after a wallet switch could therefore consume values fetched for the former wallet.
2. User-entered and Max-derived amounts were not scope-bound
The deposit input and exact raw Max value persisted independently from the active wallet scope:
Invalidating only the balance snapshot did not invalidate an amount already derived from that balance.
3. Deposit validation was incomplete while balance was pending
The over-deposit check can only compare values when the active wallet balance is known:
While the replacement balance remained unresolved, that comparison was intentionally unavailable. Without a separate fail-closed guard, a stale raw Max amount could remain eligible for submission.
Implementation
1. Introduce a wallet-scoped balance snapshot
The component now stores balance data together with the ownership scope that produced it:
The nullable
amountallows the component to represent a known active scope whose balance is still pending or unavailable without reusing data from another scope.2. Derive a scope from wallet and collateral mint
The scope key is derived from both active identity components:
This prevents balance data for:
from being treated as current.
3. Expose balance state only for a matching scope
The rendered balance is derived only when the stored snapshot belongs to the currently active wallet/mint scope:
Decimals are protected by the same scope comparison.
This provides a render-time ownership check in addition to the asynchronous request lifecycle guards.
Even before an effect finishes processing the wallet change, a snapshot belonging to wallet A cannot be consumed while wallet B is active because its
scopeKeyno longer matches.4. Invalidate cross-scope balance state
When a request begins for a different wallet or collateral mint, the component replaces the previous snapshot with a pending snapshot owned by the new scope:
This prevents the previous wallet's:
from leaking into the replacement wallet lifecycle.
Same-scope post-transaction refreshes retain the already verified value while the refreshed request is pending.
5. Bind asynchronous requests to their originating scope
Each balance request captures the scope that initiated it:
A successful response is stored together with that exact scope:
A response from an obsolete request may still complete asynchronously, but it cannot become consumable under a different active scope.
6. Ignore cancelled request executions
Each effect execution retains a cancellation flag:
Cleanup marks the request execution as cancelled:
Successful and failed requests update state only while their effect execution remains active.
A failed current-scope request leaves the scope in a fail-closed state with:
It does not restore or reuse a previously verified balance from another wallet.
7. Clear visible and raw Max state on scope changes
The follow-up fix binds the selected deposit amount to the wallet/mint lifecycle:
When either the wallet public key or collateral mint changes:
Maxis cleared;Both representations must be cleared because
handleSubmitintentionally prioritizesmaxRawRef.currentto preserve exact token precision.8. Fail closed while the active balance is unverified
The component derives an explicit pending/unverified condition for deposits:
The submit handler refuses to proceed while that condition is true:
The Deposit button uses the same guard:
This creates two layers of protection:
The handler-level guard is required even though the input is cleared by an effect because it closes any timing window between the scope-changing render and effect execution.
9. Preserve exact Max precision after verification
Once the active wallet's balance resolves, the existing Max behavior remains unchanged:
The displayed amount may be formatted for human readability, while submission continues to use the exact native-unit
bigint.The difference is that the raw value is now valid only for the active wallet/mint lifecycle and is cleared whenever that lifecycle changes.
10. Preserve valid same-scope refresh behavior
Post-transaction balance refreshes triggered through
lastSigcontinue to operate normally.When the wallet and collateral mint remain unchanged:
walletBalanceScopeKeyremains stable;Regression tests
Added and extended:
Test 1: stale account-creation state is withheld while wallet B is pending
The test reproduces the new-account race:
900 USDC.Create Trading Accountis available.Create Trading Accountis no longer available.initUser.500 USDCstarter-deposit cap is not derived from wallet A while wallet B remains unverified.This test fails against the vulnerable implementation because wallet A's balance and account-creation action remain available after wallet B becomes active.
Test 2: the component updates after wallet B resolves
The second test verifies the successful replacement lifecycle:
900 USDC.25 USDC.This confirms the fix does not permanently suppress balance state. It withholds data only until the active wallet's balance has been verified.
Test 3: a Max-derived amount cannot cross wallet scopes
The third test covers the existing trading-account deposit path:
Render an existing trading account under wallet A.
Resolve wallet A's balance as
900 USDC.Click
Max.Confirm the input displays
900.Switch the active wallet to wallet B.
Keep wallet B's balance request pending.
Confirm the previous input value is cleared.
Confirm the Deposit button is disabled.
Attempt to click Deposit.
Confirm
deposit()is not called.Resolve wallet B's balance as
25 USDC.Click
Maxagain.Confirm the input displays
25.Submit the deposit.
Confirm the submitted amount is exactly:
Confirm the submission uses wallet B's portfolio identity.
This verifies that both the visible amount and the exact raw Max value are invalidated when the wallet scope changes.
Validation
Focused regression suite
pnpm --filter @percolator/app exec vitest run \ __tests__/components/trade/DepositWithdrawCard.wallet-switch.test.tsx \ --reporter=verboseResult:
The focused suite validates:
TypeScript validation
pnpm --filter @percolator/app exec tsc --noEmitResult:
Production build
Result:
Repository whitespace validation
Result:
Formatting note
A full-file Prettier check reports existing formatting differences in:
The same warning is reproducible against the previous PR HEAD before the review follow-up patch.
The follow-up therefore intentionally preserves a minimal behavioral diff instead of reformatting the entire component and introducing unrelated formatting churn.
The regression-test file itself does not produce the reported formatting warning.
GitHub Actions
The latest PR head passes:
Files changed
User impact
After this change, switching wallets cannot temporarily expose or reuse the previous wallet's collateral balance inside the trade deposit/account-creation card.
For a replacement wallet:
This prevents stale client-side transaction intent from crossing wallet or collateral-mint ownership boundaries.
Scope
This change is intentionally limited to the wallet-balance and deposit-input lifecycle inside
DepositWithdrawCard.It does not alter:
The patch ensures that client-side balance data and balance-derived deposit values remain bound to the wallet and collateral mint that produced them.