You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
chore(ci): migrate the local test network from dfx and snsdemo to icp-cli - #8123
snsdemo is shutting down, see dfinity/snsdemo#628. It runs on dfx, which is deprecated and cannot run IC commits newer than 2026-10-01, so the snapshot that CI and local development depend on will stop updating. This moves the local test network to icp-cli, the same way as dfinity/governance-app#498. The release scripts keep dfx, because dfx-orbit needs it.
The icp-cli network launcher starts PocketIC with the NNS, the SNS, Internet Identity, the ICP ledger, the SNS aggregator and an nns-dapp. It has no SNS project and no ck tokens, so the end-to-end tests that need them are skipped for now.
Changes
Added icp.yaml, scripts/install-icp-cli and scripts/icp-network-start, which starts the network, makes the icp identity a controller of the nns-dapp and the aggregator through the PocketIC REST API, stakes a large NNS neuron, deploys a mock ICP Swap price feed and installs the given wasms.
Replaced the start_dfx_snapshot and checkout_snsdemo actions and the snsdemo update workflow with a start_icp_network action, and ported the build jobs, the test scripts and the Dockerfile from dfx to icp.
Added scripts/canister-id, so config.sh reads the canister IDs from the icp-cli mappings and dfx.json with jq instead of dfx.
Rewrote the e2e login for the new Internet Identity, staked 200 ICP for the dummy proposals, made the page objects wait for the next view after a back navigation, and skipped the five tests that need an SNS, ckBTC or a test token.
Fixed the SNS aggregator to serve an empty latest list with zero SNSs, and the frontend to accept an empty project list, to load the staking rewards without SNS projects, and to map the IC0542 error code.
…-cli
The snsdemo repository is shutting down, because dfx is deprecated and
cannot run new IC commits. The local network now runs on icp-cli, which
starts PocketIC with the NNS, the SNS, Internet Identity, the ICP ledger,
the SNS aggregator and an nns-dapp. A script adds the icp identity as a
controller of the nns-dapp and the aggregator, so CI can install the
build under test.
The release scripts keep dfx, because dfx-orbit needs it.
The local NNS has one neuron of 1 ICP, so a test neuron decides every
proposal at once. The start script now stakes a large neuron for the
anonymous principal. The reject cost is 25 ICP, so the dummy proposals
helper stakes 200 ICP. PocketIC answers canister_status with IC0542.
The page objects wait for the next view after a back navigation.
goBack did not await the removal of the view, so the next navigation
clicked back twice. The deep JSON test now also matches the script URL
with a query string, which the Vite dev server adds.
…oad the rewards without SNS
The upgrade jobs use migration-test and estimate-upgrade-cycles, which
still called dfx. The Docker configurator stages run config.sh, which
needs scripts/canister-id. The aggregator API accepts an empty project
list only when the first page loaded. The staking rewards waited for SNS
neurons that never load on a network without SNS projects.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The cross-cutting network migration needs human review backed by successful bootstrap, upgrade, and browser-test validation.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Add regression tests for latest-list initialization and preservation
rs/sns_aggregator/src/state.rs:295
Please add a regression test for creating the empty latest-list asset, including a check that calling setup with an existing nonempty latest list leaves it unchanged. The current state tests only exercise serialization, and the aggregator integration job in .github/workflows/build.yml is disabled, so neither the new zero-SNS initialization behavior nor its post-upgrade preservation behavior is currently verified.
Not necessary. The asset insert calls the IC system API (time and certified data), so a native unit test traps. The e2e tests cover the empty list on the local network.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The network and authentication migration needs human integration validation, particularly with five end-to-end tests disabled and unresolved setup and deployment-summary issues.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The PocketIC launcher migration requires integration validation of startup/restart behavior, controller impersonation, upgrade/downgrade tests, and Internet Identity e2e flows that could not be exercised in the review environment.
The reason will be displayed to describe this comment to others. Learn more.
scripts/nns-dapp/upgrade-downgrade-test:39–48: This now reuses an existing development network, reinstalls nns-dapp, then stops the whole network and discards its state. Refuse pre-existing networks by default, require an explicit destructive opt-in, or run the test on a network it owns.
Optional — scripts/icp-network-start:52: If nns-dapp-local already exists while anonymous is selected, startup tries to create it again and fails. Reuse the existing identity before creating it.
The upgrade-downgrade test refuses a running local network, unless
the caller passes --reuse_network. CI passes the flag, because the
network action starts the network first. The test stops the network
only when it started it.
The network start script reuses the nns-dapp-local identity when it
exists and the default identity is anonymous.
Done. The test refuses a running network unless you pass --reuse_network, and it stops only the network it started. The start script reuses the identity.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The runtime, PocketIC bootstrap, and Internet Identity migration requires live startup, upgrade/downgrade, and retained E2E validation because static review cannot establish compatibility across these integration boundaries.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Reusing an already-running network on a fresh icp-cli home leaves the newly created nns-dapp-local identity unfunded, causing mock creation to fail with insufficient cycles.
icp-cli gives cycles to the identities that exist when the network
starts. An identity created later has none, so the start script could
not create the mock canisters on a network that ran already. The script
now transfers cycles from the anonymous identity when the balance of
the current identity is low.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because setup behavior on initialization and upgrade lacks regression coverage for empty SNS state and preservation of existing latest assets, while the aggregator integration job is disabled.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The new initialization behavior lacks regression tests for an SNS-free state and preservation of an existing nonempty latest list after an upgrade, while the aggregator_test workflow job is disabled.
Two unit tests check `State::setup`. One checks that a network without an
SNS gets an empty latest list. One checks that an upgrade keeps a list that
exists. The canister time and the certification call panic outside a
canister, so unit tests get a fixed time and skip the certification.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The coupled launcher, PocketIC controller, staking, and Internet Identity migration requires live-network and fresh-network CI validation that source-only review cannot provide.
The reason will be displayed to describe this comment to others. Learn more.
TBH, I didn't read most of this carefully, but I asked Claude to check this out, and it seems alright.
I'm assuming that the disabled tests will be re-enabled in the not too distant future, particularly, since they have TODOs.
This is a pretty large PR. I think a couple of PRs could have been split out:
fixes to ./frontend
fixes to sns_aggregator
I guess those would have been pretty small though, so splitting those out would not really make this much smaller. To be clear, I'm not asking you to split those out now. Just saying, maybe consider this in future PRs.
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
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.
artkorotkikh-dfinity
left a comment
daniel-wong-dfinity-org-twin
left a comment
Motivation
snsdemo is shutting down, see dfinity/snsdemo#628. It runs on dfx, which is deprecated and cannot run IC commits newer than 2026-10-01, so the snapshot that CI and local development depend on will stop updating. This moves the local test network to icp-cli, the same way as dfinity/governance-app#498. The release scripts keep dfx, because dfx-orbit needs it.
The icp-cli network launcher starts PocketIC with the NNS, the SNS, Internet Identity, the ICP ledger, the SNS aggregator and an nns-dapp. It has no SNS project and no ck tokens, so the end-to-end tests that need them are skipped for now.
Changes
icp.yaml,scripts/install-icp-cliandscripts/icp-network-start, which starts the network, makes the icp identity a controller of the nns-dapp and the aggregator through the PocketIC REST API, stakes a large NNS neuron, deploys a mock ICP Swap price feed and installs the given wasms.start_dfx_snapshotandcheckout_snsdemoactions and the snsdemo update workflow with astart_icp_networkaction, and ported the build jobs, the test scripts and the Dockerfile from dfx to icp.scripts/canister-id, soconfig.shreads the canister IDs from the icp-cli mappings anddfx.jsonwith jq instead of dfx.latestlist with zero SNSs, and the frontend to accept an empty project list, to load the staking rewards without SNS projects, and to map the IC0542 error code.