fix(sns): read the swap participant count only from the certified state - #8049
Open
yhabib wants to merge 5 commits into
Open
fix(sns): read the swap participant count only from the certified state#8049yhabib wants to merge 5 commits into
yhabib wants to merge 5 commits into
Conversation
The project detail page read the participant count from `https://<swap>.raw.icp0.io/metrics` when `get_derived_state` returned no `direct_participant_count`. The raw gateway skips response certification, so one replica could answer with any count. Delete the raw metrics path. `direct_participant_count` from the certified `get_derived_state` call is now the only source of the count.
The spec opens a sale that accepts participation, reads the participant count, participates, and checks that the count rises by one. It also records every network request and checks that no request goes to a raw metrics URL.
…spec step() is async; a call without await lets Playwright run the next step before the report closes the previous one.
|
✅ No security or compliance issues detected. Reviewed everything up to 4ce6e46. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A newly added unit-test assertion relies on global.fetch being a Vitest mock but it is not stubbed in the test setup, which can cause the test to fail/throw before validating the behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes the uncertified raw /metrics fallback for SNS swap participant counts and ensures the UI only displays the count when it is present in the certified get_derived_state response, closing a spoofing vector from the raw gateway.
Changes:
- Removed the raw
https://<swap>.raw.icp0.io/metricsfetch path (API/service/store/util parsing) and associated watcher branches. - Updated swap participant-count derivation to read only
direct_participant_countfrom the certified derived state, and fail-closed when missing. - Added/updated tests (unit + e2e) and added a Security changelog entry to prevent regressions.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/utils/sns-swap.utils.ts | Drops raw-metrics fallback; reads participant count only from certified derived state. |
| frontend/src/lib/pages/ProjectDetail.svelte | Removes raw-metrics watcher/fetch branch and related imports. |
| frontend/src/lib/components/project-detail/ProjectCommitment.svelte | Removes metrics-store dependency; renders participant count only when certified field exists. |
| frontend/src/lib/utils/sns.utils.ts | Removes raw-metrics parsing helper used by the deleted fallback path. |
| frontend/src/lib/api/sns-swap-metrics.api.ts | Deleted raw /metrics API client. |
| frontend/src/lib/services/sns-swap-metrics.services.ts | Deleted service that fetched/parses/stores raw metrics. |
| frontend/src/lib/stores/sns-swap-metrics.store.ts | Deleted store that cached raw metrics per SNS root canister. |
| frontend/src/tests/lib/pages/ProjectDetail.spec.ts | Updates expectations to assert no raw-domain fetch is performed. |
| frontend/src/tests/lib/components/project-detail/ProjectCommitment.spec.ts | Adds fail-closed assertions when derived participant count is missing. |
| frontend/src/tests/lib/utils/sns-swap.utils.spec.ts | Updates utils tests to cover certified-only behavior and missing-field semantics. |
| frontend/src/tests/lib/utils/sns.utils.spec.ts | Removes tests for deleted raw-metrics parsing helper. |
| frontend/src/tests/lib/api/sns-swap-metrics.api.spec.ts | Deleted tests for removed raw-metrics API. |
| frontend/src/tests/lib/services/sns-swap-metrics.services.spec.ts | Deleted tests for removed raw-metrics service. |
| frontend/src/tests/lib/stores/sns-swap-metrics.store.spec.ts | Deleted tests for removed raw-metrics store. |
| frontend/src/tests/page-objects/ProjectCommitment.page-object.ts | Adds a helper to assert the participant count row exists. |
| frontend/src/tests/e2e/sns-swap-participant-count.spec.ts | New e2e regression spec ensuring no raw metrics requests occur and count increases after participating. |
| CHANGELOG-Nns-Dapp-unreleased.md | Documents the security fix under the Security section. |
Review details
- Files reviewed: 17/17 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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A page-object helper used to assert absence of the “minimum reached” banner can still time out/throw in Playwright when the element is missing, so it should be adjusted to return null safely and consistently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
getGoalReachedMessage called getText, which times out in the Playwright backend when the element is not present. Check isPresent first and return null, so the helper behaves the same in Jest and Playwright.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new e2e test can be flaky under parallel Playwright workers because other tests also participate in the same sale, so the participant count may increase by more than one.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/sns-swap-participant-count.spec.ts:78
- This e2e assertion expects the participant count to increase by exactly 1 after participating. Because Playwright is configured with multiple workers (frontend/playwright.config.ts sets
workers: 6) and other e2e specs (e.g.sns-participation.spec.ts) also participate in the same sale, this can be flaky when another worker participates concurrently and the count jumps by >1. Prefer asserting the count increases by at least 1 (or make the suite serial / isolate the sale).
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Another e2e file participates in the same first upcoming sale, so a worker running both files at once can raise the count by more than one. Check for at least one more, not exactly one more.
Contributor
Author
|
Fixed: the e2e count assertion now checks for at least one more participant ( |
Contributor
There was a problem hiding this comment.
🟢 Approved
The raw, uncertified metrics pathway is fully removed (code + imports + tests), and new unit/e2e coverage verifies the app no longer makes raw metrics requests while failing closed when the certified field is absent.
Review details
- Files reviewed: 17/17 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 SNS project detail page is where a user decides to join a sale. When the derived swap state carried no participant count, the page fetched it from
https://<swap>.raw.icp0.io/metrics, a raw domain with no response certification. A single replica on the swap subnet could answer that request with a forged count and mislead a viewer about how close the sale is to succeeding.Every live SNS swap canister already reports
direct_participant_countthrough the certifiedget_derived_statecall, so the raw fallback had no real target left. Only one aborted swap (SONIC) still lacked the field, and its swap canister holds no wasm, so the raw fetch answered nothing there too.Changes
/metricsfetch and its api, service, and store modules.swapSaleBuyerCountto read the count only from the certifieddirect_participant_countfield.ProjectDetail.svelteandProjectCommitment.svelte.Tests
ProjectDetail.spec.tsthat fails onmain(the raw fetch fires) and passes on this branch.ProjectCommitment.spec.tsandsns-swap.utils.spec.ts: an emptydirect_participant_countshows no count and no "minimum reached" banner, instead of a false zero.sns-swap-participant-count.spec.ts, that records every network request, participates in a live sale, and checks the raw domain is never called while the participant count still rises by one.npm run check,CI=true npm run test, and./scripts/check-relative-importsall pass.Todos
fix/certified-balance-refresh) covers the balances worker, PR fix(tokens): confirm portfolio token balances with certified calls #8048 (fix/portfolio-balances-certified) covers the main-thread portfolio services. This PR and fix(tokens): confirm portfolio token balances with certified calls #8048 both add a line under the changelog's Security section, so whoever merges second should rebase and keep both lines.