Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(sns): Porting sns-testing to the ICP mono repo #3979

Merged
merged 12 commits into from
Feb 17, 2025
Merged
25 changes: 25 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ members = [
"rs/sns/swap",
"rs/sns/swap/proto_library",
"rs/sns/test_utils",
"rs/sns/testing",
"rs/starter",
"rs/state_manager",
"rs/state_machine_tests",
Expand Down
2 changes: 2 additions & 0 deletions packages/pocket-ic/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Unreleased

### Added
- The function `PocketIc::try_get_controllers` which gets the controllers of a canister but doesn't panic if the target canister
doesn't exist.
- The function `PocketIcBuilder::with_bitcoind_addrs` to specify multiple addresses and ports at which `bitcoind` processes are listening.
- The function `PocketIc::query_call_with_effective_principal` for making generic query calls (including management canister query calls).
- The function `PocketIc::ingress_status` to fetch the status of an update call submitted through an ingress message.
Expand Down
12 changes: 11 additions & 1 deletion packages/pocket-ic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ use candid::{
Principal,
};
use ic_transport_types::SubnetMetrics;
use reqwest::Url;
use reqwest::{StatusCode, Url};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use slog::Level;
Expand Down Expand Up @@ -649,6 +649,16 @@ impl PocketIc {
runtime.block_on(async { self.pocket_ic.get_controllers(canister_id).await })
}

/// Get the controllers of a canister.
#[instrument(ret, skip(self), fields(instance_id=self.pocket_ic.instance_id, canister_id = %canister_id.to_string()))]
pub fn try_get_controllers(
&self,
canister_id: CanisterId,
) -> Result<Vec<Principal>, (StatusCode, String)> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not check if the canister exists via a separate call? Also returning the HTTP status code in this API looks strange.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rvem rvem Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to assert that it is "not found"

Disregard, the server returns 400 CanisterNotFound(CanisterId(rrkah-fqaaa-aaaaa-aaaaq-cai)) when canister doesn't exist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how do you retrieve canister_info using a "deterministic" PocketIC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let runtime = self.runtime.clone();
runtime.block_on(async { self.pocket_ic.try_get_controllers(canister_id).await })
}

/// Get the current cycles balance of a canister.
#[instrument(ret, skip(self), fields(instance_id=self.pocket_ic.instance_id, canister_id = %canister_id.to_string()))]
pub fn cycle_balance(&self, canister_id: CanisterId) -> u128 {
Expand Down
15 changes: 12 additions & 3 deletions packages/pocket-ic/src/nonblocking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,16 +519,25 @@ impl PocketIc {
/// Panics if the canister does not exist.
#[instrument(ret, skip(self), fields(instance_id=self.instance_id, canister_id = %canister_id.to_string()))]
pub async fn get_controllers(&self, canister_id: CanisterId) -> Vec<Principal> {
self.try_get_controllers(canister_id).await.unwrap()
}

/// Get the controllers of a canister.
#[instrument(ret, skip(self), fields(instance_id=self.instance_id, canister_id = %canister_id.to_string()))]
pub async fn try_get_controllers(
&self,
canister_id: CanisterId,
) -> Result<Vec<Principal>, (StatusCode, String)> {
let endpoint = "read/get_controllers";
let result: Vec<RawPrincipalId> = self
.post(
let result: Result<Vec<RawPrincipalId>, (StatusCode, String)> = self
.try_post(
endpoint,
RawCanisterId {
canister_id: canister_id.as_slice().to_vec(),
},
)
.await;
result.into_iter().map(|p| p.into()).collect()
result.map(|v| v.into_iter().map(|p| p.into()).collect())
}

/// Get the current cycles balance of a canister.
Expand Down
13 changes: 13 additions & 0 deletions packages/pocket-ic/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1739,6 +1739,19 @@ fn get_controllers_of_nonexisting_canister() {
let _ = pic.get_controllers(canister_id);
}

#[test]
fn try_get_controllers_of_nonexisting_canister() {
let pic = PocketIc::new();

let canister_id = pic.create_canister();
pic.add_cycles(canister_id, 100_000_000_000_000);
pic.stop_canister(canister_id, None).unwrap();
pic.delete_canister(canister_id, None).unwrap();

let res = pic.try_get_controllers(canister_id);
assert!(res.is_err())
}

#[test]
fn test_canister_snapshots() {
let pic = PocketIc::new();
Expand Down
2 changes: 1 addition & 1 deletion rs/ledger_suite/icp/index/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use serde_bytes::ByteBuf;

pub mod logs;

#[derive(Debug, CandidType, Deserialize)]
#[derive(Clone, Debug, CandidType, Deserialize)]
pub struct InitArg {
pub ledger_id: Principal,
}
Expand Down
6 changes: 5 additions & 1 deletion rs/nervous_system/agent/src/pocketic_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,11 @@ impl CallCanisters for PocketIcAgent<'_> {
) -> Result<CanisterInfo, Self::Error> {
let canister_id = canister_id.into();

let controllers = self.pocket_ic.get_controllers(canister_id).await;
let controllers = self
.pocket_ic
.try_get_controllers(canister_id)
.await
.unwrap_or(vec![]);

let Some(controller) = controllers.into_iter().last() else {
return Err(Self::Error::BlackHole);
Expand Down
Loading
Loading