test(sns): Porting sns-testing to the ICP mono repo #3979
Conversation
Canister installation may fail due to insufficient cycles, so add cycles before installing the canister in 'install_canister_with_controllers'. The same is already done in 'install_canister_on_subnet'.
Problem: NNS dapp needs ICP ledger index and Cycles minting canisters to be deployed. Currently 'install_nns_canisters' doesn't install them. Solution: Add 'install_nns_suite' helper that optionally installs ICP ledger index and Cycles minting canisters. The existing 'install_nns_canisters' reuses 'install_nns_suite' but doesn't install them.
Init the new sns-testing project. Currently, this crate provides CLI to bootstrap NNS on the provided PocketIC instance.
Co-authored-by: Arshavir Ter-Gabrielyan <[email protected]>
1) Add a simple test canister that will be decentralized as a part of newly created SNS 2) Add helpers to install this canister and run it as the new SNS 3) Complete the SNS swap to finalize SNS adoption
Add helpers to upgrade the test canister. Currently, the helper will reuse the same WASM module, but it's possible to provide the new state value as a 'post_upgrade' hook argument. Test scenario as well as CLI now perform canister upgrade. The former also allows to see the SNS voting procedure in the NNS dapp web UI.
Problem: Currently, helpers from 'pocket_ic_helpers' module use 'advance_time' pocket-ic method to artificially advance time in order to wait for certain events to happen. However, using 'advance_time' in live mode breaks agent certificates checking since pocket-ic time becomes larger than the real time. Solution: 1) When pocket-ic is running in live mode, run 'std::thread::sleep()' instead of 'advance_time()'. 2) Avoid waiting two days for SNS swap to start in live mode. 3) Allow to use the test version of NNS Governance canister which is capable of starting SNS swap immediately after SNS proposal adoption.
Problem: We want to be able to run pocket-ic in live mode. However, this is not viable with non-test version of NNS Governance canister because it postpones the SNS swap start. Solution: Use test version of NNS Governance canister. Ensure that swap start_time is 'None' to start the swap immediately after SNS creation proposal adoption.
Check that PocketIC network has all required subnets and install NNS-related canisters (core and frontend) only when they're missing. At this point, we do not check that canisters were correctly initialized, so this check might not be sufficient to run the scenario on arbitrary network.
'get_controllers' method panics if the target canister doesn't exist
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
aterga
dismissed
github-actions[bot]’s stale review
No canister behavior changes.
aterga
changed the title
There was a problem hiding this comment.
I concur that the changes here are the same as #3654 (as of 10 minutes ago or so). If this is sufficient to merge, please do.
This PR implements the next-gen [sns-testing](https://github.com/dfinity/sns-testing) tool. It can now be written in Rust and continuously tested against the latest SNS features. At the core of this tool lies the [PocketIC framework](https://crates.io/crates/pocket-ic/6.0.0), the testing framework capable of creating a fully deterministic environment for testing complex ICP dapp scenarios. This PR is authored by Serokell and reviewed by the DFINITY Foundation. Please refer to the reviews here: #3654 --------- Co-authored-by: Roman Melnikov <[email protected]> Co-authored-by: Roman Melnikov <[email protected]>
| pub fn try_get_controllers( | ||
| &self, | ||
| canister_id: CanisterId, | ||
| ) -> Result<Vec<Principal>, (StatusCode, String)> { |
There was a problem hiding this comment.
Why not check if the canister exists via a separate call? Also returning the HTTP status code in this API looks strange.
There was a problem hiding this comment.
Why not check if the canister exists via a separate call?
The behavior of canister_info which panics when the target canister doesn't exist was strange for me.
The panic is caused by the fact that canister_info used get_controllers which also panics when canister doesn't exist, so I decided to make it return some error instead of panicking
Also returning the HTTP status code in this API looks strange
Agree
There was a problem hiding this comment.
Also returning the HTTP status code in this API looks strange
Perhaps, the proper way to handle the error code is to assert that it is "not found" returning human-readable error via String and panic otherwise
There was a problem hiding this comment.
to assert that it is "not found"
Disregard, the server returns 400 CanisterNotFound(CanisterId(rrkah-fqaaa-aaaaa-aaaaq-cai)) when canister doesn't exist
There was a problem hiding this comment.
Does smth like
diff --git a/packages/pocket-ic/src/lib.rs b/packages/pocket-ic/src/lib.rs
index f84553dc1a..ad2f61e5ef 100644
--- a/packages/pocket-ic/src/lib.rs
+++ b/packages/pocket-ic/src/lib.rs
@@ -654,7 +654,7 @@ impl PocketIc {
pub fn try_get_controllers(
&self,
canister_id: CanisterId,
- ) -> Result<Vec<Principal>, (StatusCode, String)> {
+ ) -> Result<Vec<Principal>, String> {
let runtime = self.runtime.clone();
runtime.block_on(async { self.pocket_ic.try_get_controllers(canister_id).await })
}
diff --git a/packages/pocket-ic/src/nonblocking.rs b/packages/pocket-ic/src/nonblocking.rs
index 84d04a1f14..a9915bfc28 100644
--- a/packages/pocket-ic/src/nonblocking.rs
+++ b/packages/pocket-ic/src/nonblocking.rs
@@ -527,7 +527,7 @@ impl PocketIc {
pub async fn try_get_controllers(
&self,
canister_id: CanisterId,
- ) -> Result<Vec<Principal>, (StatusCode, String)> {
+ ) -> Result<Vec<Principal>, String> {
let endpoint = "read/get_controllers";
let result: Result<Vec<RawPrincipalId>, (StatusCode, String)> = self
.try_post(
@@ -537,7 +537,17 @@ impl PocketIc {
},
)
.await;
- result.map(|v| v.into_iter().map(|p| p.into()).collect())
+ match result {
+ Ok(result) => Ok(result.into_iter().map(|p| p.into()).collect()),
+ Err((status, message)) => {
+ assert_eq!(
+ StatusCode::BAD_REQUEST,
+ status,
+ "HTTP code {status} for read/get_controllers is not BAD_REQUEST, got: {message}"
+ );
+ Err(format!("Failed to get controllers for canister {canister_id}: {message}"))
+ },
+ }
}
/// Get the current cycles balance of a canister.
make sense to you?
There was a problem hiding this comment.
I see: so you're using an agent to interact with a "live" PocketIC instance. Most PocketIC library APIs (such as get_controllers are supposed to work in the non-live mode where the instance does not necessarily make progress on its own).
There was a problem hiding this comment.
I see: so you're using an agent to interact with a "live" PocketIC instance
Not necessarily, the goal is to be agnostic of client "backend" implementation (live/deterministic PocketIC or ic-agent)
There was a problem hiding this comment.
So how do you retrieve canister_info using a "deterministic" PocketIC?
This PR implements the next-gen sns-testing tool. It can now be written in Rust and continuously tested against the latest SNS features.
At the core of this tool lies the PocketIC framework, the testing framework capable of creating a fully deterministic environment for testing complex ICP dapp scenarios.
This PR is authored by Serokell and reviewed by the DFINITY Foundation. Please refer to the reviews here: #3654