fix(frontend): stop the transactions worker from paging forever - #8050
Open
yhabib wants to merge 3 commits into
Open
fix(frontend): stop the transactions worker from paging forever#8050yhabib wants to merge 3 commits into
yhabib wants to merge 3 commits into
Conversation
The transactions worker asked the Index canister for the next page of transactions in a recursion. Nothing capped the number of pages, and nothing checked that a page moved the oldest transaction id down. An Index canister that ignores "start" made the worker fetch pages without an end. The worker memory and the emitted payload grew until the tab froze. Rewrite getIcrcAccountTransactions as a loop. - Stop after DEFAULT_INDEX_TRANSACTION_MAX_PAGES pages in one sync. - Stop when a page does not move the oldest transaction id below the start of the call that produced it. - Collect the pages and flatten them once, which removes the O(n^2) copy of the previous unwind.
…tion The spec watches the index canister traffic of an imported token. It groups the get_account_transactions requests into syncs and checks that no sync sends more than DEFAULT_INDEX_TRANSACTION_MAX_PAGES requests.
|
✅ No security or compliance issues detected. Reviewed everything up to 09ee557. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new loop’s oldestTxId computation sorts bigint IDs using Number(...), which can overflow/lose precision and undermine the pagination stop/progress logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the ICRC transactions web worker against hostile/misbehaving index canisters by ensuring pagination cannot run indefinitely (preventing unbounded memory growth during sync).
Changes:
- Reworked ICRC index pagination from recursion to an iterative loop with a “forward progress” check and a hard per-sync page cap.
- Added unit tests covering repeated pages, slow progress, and multi-page pagination behavior.
- Added a Playwright e2e spec that verifies index-canister request bursts stay within the configured page cap, plus a Security changelog entry.
File summaries
| File | Description |
|---|---|
frontend/src/lib/worker-services/icrc-transactions.worker-services.ts |
Replaces recursive paging with a bounded loop and adds progress/cap stopping conditions. |
frontend/src/lib/constants/constants.ts |
Introduces DEFAULT_INDEX_TRANSACTION_MAX_PAGES to cap pages per sync. |
frontend/src/tests/lib/worker-services/icrc-transactions.worker-services.spec.ts |
Adds unit tests for hostile/no-progress pages, max-pages cap, and correct start chaining. |
frontend/src/tests/e2e/transactions-worker-pagination.spec.ts |
Adds an e2e check that sync bursts do not exceed the max-pages cap. |
CHANGELOG-Nns-Dapp-unreleased.md |
Documents the security hardening of transactions sync pagination. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The pagination loop found the oldest transaction id with a sort comparator that converted a bigint difference to Number. A hostile index canister can answer an id above Number.MAX_SAFE_INTEGER, and that conversion loses precision. A wrong oldest id can pick a wrong result and defeat the progress and page-cap checks. Find the oldest id with a reduce that compares the bigint ids directly. No Number conversion, no lost precision.
Contributor
There was a problem hiding this comment.
🟢 Approved
The termination/cap logic and test coverage align with the stated security motivation, and the remaining findings are minor documentation wording/accuracy nits.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
frontend/src/lib/constants/constants.ts:10
- The trade-off comment says the UI will show a gap "until the user reloads the page", but older transactions can also be fetched via the existing "load next/older transactions" path (pagination). Consider wording that doesn’t imply a full reload is required.
frontend/src/lib/worker-services/icrc-transactions.worker-services.ts:139 - The comment refers to a non-existent/incorrect constant name (
DEFAULT_ICRC_TRANSACTION_PAGE_LIMIT); this file usesDEFAULT_INDEX_TRANSACTION_PAGE_LIMIT, so the comment is misleading.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
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 transactions worker pages through the index canister until a page stops giving new transaction IDs. It trusts the oldest ID from each page, with no limit and no check for forward progress. A hostile index canister behind an imported token can return pages that never end. The worker loop then never stops, and memory grows without a bound.
Changes
getIcrcAccountTransactionsfrom recursion to a loop, and flattened the fetched pages once instead of on each unwind.DEFAULT_INDEX_TRANSACTION_MAX_PAGES, at 10 pages per sync.Tests
npm run checkinfrontend: pass, 0 errors and 0 warnings.CI=true npm run testinfrontend: pass, 5908 tests.Todos