fix(tokens): confirm portfolio token balances with certified calls - #8048
fix(tokens): confirm portfolio token balances with certified calls#8048yhabib wants to merge 4 commits into
Conversation
The Portfolio, Tokens and Staking pages loaded every SNS and ICRC token balance with a query call only. A single replica could answer with a forged balance, and no certified call ever replaced it. Both balance services now use the default query-and-update strategy. The query answer still shows first and the certified answer replaces it. Rename the two services to match the verb this code base uses for a query-and-update load.
The spec signs in a new user, opens the Portfolio page and records every request. It asserts that each ledger which gets an icrc1_balance_of query call also gets an icrc1_balance_of update call.
|
✅ No security or compliance issues detected. Reviewed everything up to d8d4ac7. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
🟡 Changes recommended
The new e2e test uses the async step() helper without awaiting it, and the new ICRC balance service uses an SNS-specific toast key, both of which should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the Portfolio/Tokens/Staking initial balance display by switching token balance loading to a query-then-certified-follow-up flow, preventing a malicious replica from leaving forged balances on screen for a full session.
Changes:
- Switch SNS and ICRC/ck token balance loading to the default query-and-update strategy (and rename/move services accordingly).
- Update Portfolio/Tokens callers and unit tests to reflect the new “two calls per ledger” behavior and to assert the certified value wins.
- Add an e2e test that verifies every ledger balance query observed on the Portfolio page is followed by a certified update call.
File summaries
| File | Description |
|---|---|
| frontend/src/tests/routes/app/tokens/page.spec.ts | Updates expected balance call counts to account for query + certified update per token. |
| frontend/src/tests/routes/app/portfolio/page.spec.ts | Refactors balance call assertions and adds a unit test verifying certified balances override forged query results. |
| frontend/src/tests/lib/services/sns-accounts-balance.services.spec.ts | Adds coverage for query+update calls and for store ending with the certified balance. |
| frontend/src/tests/lib/services/icrc-accounts-balance.services.spec.ts | Renames/updates tests for the moved ICRC balance service and asserts certified outcomes/call patterns. |
| frontend/src/tests/lib/services/accounts-balances.services.spec.ts | Updates mocks/spies to the renamed balance sync services. |
| frontend/src/tests/e2e/portfolio-certified-balances.spec.ts | New e2e spec validating query calls are always paired with certified update calls for balances. |
| frontend/src/routes/(app)/(nns)/tokens/+page.svelte | Switches Tokens page balance loading to the new sync services. |
| frontend/src/lib/services/wallet-uncertified-accounts.services.ts | Removes the old query-only wallet/IERC balance loader service. |
| frontend/src/lib/services/sns-accounts-balance.services.ts | Renames SNS balance loader and drops the forced query-only strategy. |
| frontend/src/lib/services/icrc-accounts-balance.services.ts | Adds a new ICRC balance loader service that delegates to syncAccounts (query + update). |
| frontend/src/lib/services/accounts-balances.services.ts | Updates Portfolio-level balance orchestration to call the renamed sync services. |
| CHANGELOG-Nns-Dapp-unreleased.md | Adds a Security changelog entry describing the certified balance confirmation behavior. |
Review details
Suppressed comments (2)
frontend/src/tests/e2e/portfolio-certified-balances.spec.ts:73
stepshould be awaited so the Playwright step boundaries are applied as intended.
step("Every ledger that got a balance query also gets a balance update");
// On main this list keeps every queried ledger, because the Portfolio page
frontend/src/tests/e2e/portfolio-certified-balances.spec.ts:83
stepshould be awaited so the Playwright step boundaries are applied as intended.
step("No ledger gets an update call without a query call");
// The query answer must still show first, so an update call alone would mean
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Await every step() call in the certified-balances e2e spec. Use the generic error.accounts_load toast key in the ICRC balance sync service instead of an SNS-specific key.
There was a problem hiding this comment.
🔵 Needs a closer look
The new e2e spec appears to misclassify certified balance reads as /call update requests (likely missing certified query /read_state behavior), which can make the CI signal unreliable even when certification works.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/portfolio-certified-balances.spec.ts:13
- This test assumes the certified follow-up for
icrc1_balance_ofwill hit the HTTP/.../callendpoint (and documents/api/v3/.../call), butqueryIcrcBalancecallsIcrcLedgerCanister.balance(...), which is a query method. Certified queries are still sent via/.../query(typically with additional/.../read_staterequests for certification), soupdatedLedgersmay never be populated and the test can fail even when certification is working.
Consider detecting certification via read_state requests for the same canister (or by counting two icrc1_balance_of query requests per ledger, if that’s the intended behavior), rather than treating /call as the certified indicator.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes balance-fetching certification behavior on core user-facing pages and should get a final human review to validate edge cases and CI e2e behavior despite mostly solid test updates.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
State that the certified answer can arrive before the query answer, in which case the query answer is skipped.
There was a problem hiding this comment.
🟢 Approved
The changes align with the stated security goal and add targeted unit/e2e coverage, with only minor maintainability/test-strengthening follow-ups noted.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
frontend/src/lib/services/icrc-accounts-balance.services.ts:32
Promise.allSettled+ rejected-check here is unlikely to reflect ledger/balance failures becausesyncAccountsdelegates toqueryAndUpdate, which catches request errors and resolves instead of rejecting. This makes theerror.accounts_loadtoast effectively dead code for the failure mode this service is meant to report; prefer handling real exceptions per-universe with try/catch (and keep relying on the lower-level services' own toast logic for request failures).
frontend/src/tests/routes/app/tokens/page.spec.ts:1062- This assertion updates the call count, but it doesn’t actually verify that the new second call is a certified balance read (it would still pass if the code made 2 query calls). Since the comment says there is one query + one update call per token, assert the certified/non-certified split explicitly.
This issue also appears on line 1073 of the same file.
frontend/src/tests/routes/app/tokens/page.spec.ts:1076
- Same as above: this checks only total call count, not that the certified update call is present; adding explicit assertions for
certified: true/falsekeeps this test aligned with the intended security behavior.
expect(icrcLedgerApi.queryIcrcBalance).toBeCalledTimes(
// One query call and one update call per token.
2 * notFailedTokenCount
);
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Motivation
The Portfolio page is where a user reads their total balance. On the Portfolio, Tokens, and Staking pages, SNS and ck token balances loaded with an uncertified query call and no certified follow-up. A malicious replica could return a false balance that stayed on screen for the whole session.
Changes
querystrategy insns-accounts-balance.services.ts, so it uses the default query-and-update strategy.uncertifiedLoadSnsesAccountsBalancestosyncSnsAccountsBalances.wallet-uncertified-accounts.services.tstoicrc-accounts-balance.services.tsand renamed the export tosyncIcrcAccountsBalances.syncAccounts, which sends both a query and a certified update call.accounts-balances.services.tsandtokens/+page.svelteto the new names.Tests
Added unit tests that mock a query answer and a different update answer, and check the store ends up with the certified value, not the query value. Updated existing specs for the new call counts (one query plus one update per ledger, instead of one query). Added an e2e spec,
portfolio-certified-balances.spec.ts, that records network requests on the Portfolio page and checks every ledger with a balance query also gets a balance update.Ran
npm run check,CI=true npm run test, and./scripts/check-relative-importsin the worktree. All pass. The e2e spec could not run locally (no working replica on this machine) and needs a run in CI or on a machine with a replica before release.Todos
#### Security.fix/certified-balance-refresh) covers the 30 s balances worker refresh, and branchfix/sns-swap-metrics-certifiedcovers SNS swap metrics. This PR covers the first load on the Portfolio, Tokens, and Staking pages. Whichever lands last must rebase and keep every#### Securitychangelog line, not just one side.