fix(frontend): load the OISY Trade balances during initialization - #13998
Merged
Conversation
The hero net worth counts DEX-deposited balances via `oisyTradeUsdValue`, but `loadOisyTrade` only ran from the Trading tab and the OISY Trade page, so a fresh login showed a total that excluded them until one of those surfaces was visited. Add `LoaderOisyTrade` to the app-wide `Loaders` tree, mirroring `LoaderLiquidium` (the Earn data), so pairs, supported tokens, balances and orders load during initialization. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Identity-transition races can expose stale account data, and the loader uses the aggregate rather than provider-specific feature gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds app-wide OISY Trade loading so deposited balances are included in net worth immediately after initialization.
Changes:
- Adds a reactive OISY Trade loader.
- Mounts it in the global loader tree.
- Adds loader behavior tests.
File summaries
| File | Description |
|---|---|
LoaderOisyTrade.svelte |
Loads OISY Trade data app-wide. |
Loaders.svelte |
Mounts the new loader. |
LoaderOisyTrade.spec.ts |
Tests identity and feature-gate behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rrent `loadOisyTrade` captured the identity at call time, awaited four canister calls and then committed to `oisyTradeStore` unconditionally. Making the load app-wide put that on the sign-out path: `LoaderOisyTrade` re-runs its effect with a nullish identity and resets the store, while the in-flight load for the previous account can still resolve afterwards and write its balances back — so the hero kept summing them until the reload committed. Re-read `authIdentity` at the commit point and drop the result when the principal no longer matches. Guarding in the service rather than the loader covers the deposit, withdraw and limit-order reload paths too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…b.com/dfinity/oisy-wallet into impr/frontend/load-oisy-trade-at-init
`LoaderOisyTrade` guarded on `anyTradingProviderEnabled`, the surface-level aggregate, while everything it does is provider-specific — `loadOisyTrade` talks to the oisy_trade canister. The two are the same value today, but the codebase models the Trading surface staying reachable through another provider with OISY TRADE off, and in that configuration the loader would call the disabled provider's APIs on every initialization. Gate on `OISY_TRADE_ENABLED` instead, matching the split `TradingList` and `OisyTradeProvider` already make, and drive both flags independently in the spec so the mixed-provider configuration is covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent loads for the same identity can still commit stale balances and orders out of sequence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
The identity check compares principals only, so two loads for the same account were indistinguishable at the commit and the last one to resolve won regardless of which started first. `loadOisyTrade` has several concurrent callers — the app-wide loader, the initial load each `IntervalLoader` fires on mount, the poll, and the post-deposit / post-withdraw / post-limit-order refreshes — so a refresh triggered by an action could be overtaken by a poll that was already in flight and put the pre-action balance back on screen. Take a monotonic `loadGeneration` at entry, before the nullish-identity branch so a sign-out invalidates whatever is in flight too, and commit only when that generation is still the latest and the identity still current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The app-wide loader unnecessarily fetches up to 500 orders, increasing canister traffic and delaying the balance needed by the hero.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
The app-wide loader called the full `loadOisyTrade`, which fetches pairs, supported tokens, balances and up to five pages of order history and commits all four at once. The hero reads none of that except the balances — `oisyTradeUsdValue` derives from `oisyTradeBalances` joined against `enabledIcTokens` and `exchanges` — so every signed-in initialization made three canister queries whose results are only ever read on the Trading surfaces, and deep order histories delayed the total. Add `loadOisyTradeBalances`, fetching `getBalances` alone and committing through a new `setBalances` partial writer so it cannot blank what the Trading tab loaded. Mirrors the existing `loadOisyTradeSwapPairs`/ `setPairs` carve-out for the quote path. The page-level loaders keep the full `loadOisyTrade`, so orders stay page-level. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sbpublic
marked this pull request as ready for review
September 11, 2026 07:49
|
✅ No security or compliance issues detected. Reviewed everything up to 47fe7bd. Security Overview
Detected Code Changes
|
AntonioVentilii
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The hero net worth already counts DEX-deposited balances —
ExchangeBalanceadds$oisyTradeUsdValueto the total — butloadOisyTradewas only ever called fromTradingList(the Trading tab) andOisyTradeProvider(the OISY Trade page), both through anIntervalLoader. Until one of those surfaces was opened,oisyTradeStorestayed empty, so a freshly logged-in user saw a total that silently excluded everything they had deposited on the DEX.The Earn data does not have this problem: Liquidium loads app-wide through
LoaderLiquidiumin theLoaderstree, in addition to its page-level loader. This PR gives the trading data the same treatment.Changes
LoaderOisyTrade, an app-wide loader that loads the DEX balances reactively on the identity — a direct mirror ofLoaderLiquidium.Loadersnext toLoaderLiquidium, so the deposited balances are fetched during initialization.oisyTradeStorecommit inloadOisyTradeon the loading identity still being the current one, so a request left in flight at sign-out cannot write the previous account's balances back after the reset (review item).OISY_TRADE_ENABLEDrather than theanyTradingProviderEnabledaggregate, so it stays off when the provider is even if another one keeps the Trading surface reachable (review item).loadGenerationat the top ofloadOisyTradeand let only the newest invocation commit, so an action-triggered refresh cannot be overtaken by a poll for the same account that was already in flight (review item).loadOisyTradeBalances, which fetchesgetBalancesalone and commits through a newsetBalancespartial writer, so initialization no longer pulls pairs, supported tokens and the order history that only the Trading surfaces read (review item).The page-level
IntervalLoaders stay as they are: they keep the Trading tab and the provider page polling while visible, exactly asLiquidiumProviderdoes alongsideLoaderLiquidium.Side effect: the Trading tab no longer flashes its skeleton on first visit, since
oisyTradeLoadedis already true by the time it mounts.Tests
LoaderOisyTrade.spec.ts: loads with an identity, still calls the loader when signed out (so the store resets), and does nothing when the provider is disabled.oisy-trade.services.spec.tsfor the identity guard: an older load resolving after a sign-out, and after a second account's load — both verified to fail without the guard.LoaderOisyTrade.spec.tsnow drivesanyTradingProviderEnabledandOISY_TRADE_ENABLEDindependently, covering the mixed-provider configuration; both gate cases verified to fail against the old aggregate guard.set.npm run checkandnpm run check:testsreport no errors in the touched files.Not verified in a browser: reproducing it needs a logged-in session with an actual OISY Trade deposit.
🤖 Generated with Claude Code using Claude Opus 5 (claude-opus-5)