fix(tickers): select the most liquid ICP Swap pool for a token price - #8042
fix(tickers): select the most liquid ICP Swap pool for a token price#8042yhabib wants to merge 6 commits into
Conversation
Anybody can create an ICPSwap pool, so a token can have several pools. The provider kept the first pool with a 24h volume above 0. A new pool with one tiny trade won that selection and set the price shown. Keep the pool with the most liquidity instead. A tie goes to the higher 24h volume, then to the order of the feed. Also reject a price that is negative, not only one that is 0 or not finite.
The spec serves a feed with two ckUSDC pools. The attacker pool comes first and holds almost nothing. The spec asserts that the banner shows the price of the most liquid pool. The spec skips itself when the network configures no ICPSwap URL. dfx.json gives that URL to mainnet, app and beta only, so the local network requests no tickers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SF635YvebeCKzfF6CH8UkR
Select the most liquid pool among the pools that carry a usable price. An untraded pool reports a last_price of 0, so it gave no price when it won on liquidity alone. Fall back to the whole pair when no pool carries a usable price, which keeps the errors that the pair raised before.
|
✅ No security or compliance issues detected. Reviewed everything up to 40665b3. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
🟡 Changes recommended
The new e2e spec’s expected formatted ICP price does not match the app’s formatting behavior and will fail when executed on networks where the ICP Swap URL is configured.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves the robustness of token USD pricing derived from ICP Swap tickers by selecting the pool with the highest reported liquidity (with volume/feed-order tie-breakers) and tightening invalid-price handling, reducing susceptibility to low-liquidity “attacker” pools.
Changes:
- Updated ticker selection to choose the most liquid pool per token pair (with volume + feed-order tie-breakers) and to prefer pools with usable
last_price. - Strengthened price validation to reject non-finite, zero, and negative prices (including for the ckUSDC anchor).
- Added unit tests for attack and edge scenarios, plus a new Playwright e2e spec covering attacker-first feed ordering.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/services/icp-swap.provider.ts | Implements most-liquid pool selection and tighter price validity checks. |
| frontend/src/tests/lib/services/icp-swap.provider.spec.ts | Adds/updates unit tests for liquidity-based selection and invalid-price scenarios. |
| frontend/src/tests/e2e/icp-swap-pool-selection.spec.ts | Adds e2e coverage for attacker-first ticker ordering and most-liquid selection. |
| CHANGELOG-Nns-Dapp-unreleased.md | Documents the security-related pricing hardening. |
Review details
- Files reviewed: 4/4 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.
formatNumber replaces the decimal comma from Intl.NumberFormat("fr-FR")
with a dot, so the banner shows "4.00", not "4,00".
There was a problem hiding this comment.
🟢 Approved
The selection logic change is well-scoped and is backed by targeted unit tests and an e2e check, with only a minor test-time optimization suggested.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/icp-swap-pool-selection.spec.ts:93
- The test always waits 5 seconds even when the app requests
**/tickersimmediately, which slows down runs on networks where the ICPSwap URL is configured. You can instead wait up to 5 seconds for the tickers request and proceed as soon as it happens (and still skip when it never happens).
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
The changelog entry names ICPSwap. cargo spellcheck flagged it as a misspelling of Ipswich.
There was a problem hiding this comment.
🔵 Needs a closer look
The new e2e spec uses a fixed 5s sleep (waitForTimeout(5_000)) that can be replaced with a request-based wait to reduce runtime and flakiness.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/icp-swap-pool-selection.spec.ts:93
- Using a fixed
waitForTimeout(5_000)makes this e2e spec slower and more timing-dependent. Since you already route**/tickers, you can wait for the request (with a timeout) and then decide whether to skip, which avoids an unconditional 5s sleep when the request happens quickly.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Race the request against a 5s timeout, so the spec runs at the request's own pace on a network that sends it.
|
Fixed in 40665b3: the spec now races the tickers request against a 5s timeout, instead of a fixed 5s sleep. The request resolves a promise set inside the existing |
There was a problem hiding this comment.
🟢 Approved
The selection logic and stricter price validation align with the stated security motivation, and the added unit/e2e tests cover the key attack and edge-case scenarios.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Motivation
The USD price for a token came from the first ICP Swap pool with 24h volume above 0, in feed order. Anyone can open a new ICP Swap pool and make one small trade. That pool could then win the price selection over the real pool, so a fake pool could show a false price for ICP or any other token, including the ckUSDC anchor that scales every USD figure in the app.
Changes
liquidity_in_usdper token pair, instead of the first pool with 24h volume above 0.volume_usd_24H, then by feed order.last_price(not finite or<= 0) during the selection, so the displayed price never disappears.last_pricethat is not finite or<= 0in the price checks, instead of only=== 0.liquidity_in_usd, and a negativelast_priceon a token and on the ckUSDC anchor.Tests
npm run check,CI=true npm run test, and./scripts/check-relative-importsall pass infrontend.The provider spec (
frontend/src/tests/lib/services/icp-swap.provider.spec.ts) covers the selection rule, the attack scenario in both feed orders, and the price checks. I mutated the fix back to the oldfind(volume > 0)rule and confirmed the new tests fail without it.The e2e spec (
frontend/src/tests/e2e/icp-swap-pool-selection.spec.ts) needs theicp-swapcanister URL, whichdfx.jsononly sets formainnet,app, andbeta. It skips itself with a named reason on a local run and did not run here.Todos