feat!: change ReplicaError to Certified/UncertifiedReject (#531)

dfinity · Mar 13, 2024 · 5aaf479 · 5aaf479
1 parent 6d075ff
commit 5aaf479
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 34 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+* Changed `AgentError::ReplicaError` to `CertifiedReject` or `UncertifiedReject`. `CertifiedReject`s went through consensus, and `UncertifiedReject`s did not. If your code uses `ReplicaError`:
+    * for queries: use `UncertifiedReject` in all cases (for now)
+    * for updates: use `CertifiedReject` for errors raised after the message successfully reaches the canister, and `UncertifiedReject` otherwise
 * Added `Agent::fetch_api_boundary_nodes` for looking up API boundary nodes in the state tree.
 * Timestamps are now being checked in `Agent::verify` and `Agent::verify_for_subnet`. If you were using it with old certificates, increase the expiry timeout to continue to verify them.
 * Added node metrics, ECDSA, and Bitcoin functions to `MgmtMethod`. Most do not have wrappers in `ManagementCanister` because only canisters can call these functions.

diff --git a/ic-agent/src/agent/agent_error.rs b/ic-agent/src/agent/agent_error.rs
@@ -50,9 +50,13 @@ pub enum AgentError {
     #[error("Cannot parse Principal: {0}")]
     PrincipalError(#[from] crate::export::PrincipalError),
 
-    /// The replica rejected the message.
-    #[error("The replica returned a replica error: reject code {:?}, reject message {}, error code {:?}", .0.reject_code, .0.reject_message, .0.error_code)]
-    ReplicaError(RejectResponse),
+    /// The subnet rejected the message.
+    #[error("The replica returned a rejection error: reject code {:?}, reject message {}, error code {:?}", .0.reject_code, .0.reject_message, .0.error_code)]
+    CertifiedReject(RejectResponse),
+
+    /// The replica rejected the message. This rejection cannot be verified as authentic.
+    #[error("The replica returned a rejection error: reject code {:?}, reject message {}, error code {:?}", .0.reject_code, .0.reject_message, .0.error_code)]
+    UncertifiedReject(RejectResponse),
 
     /// The replica returned an HTTP error.
     #[error("The replica returned an HTTP Error: {0}")]

diff --git a/ic-agent/src/agent/agent_test.rs b/ic-agent/src/agent/agent_test.rs
@@ -141,7 +141,7 @@ async fn query_rejected() -> Result<(), AgentError> {
     assert_mock(query_mock).await;
 
     match result {
-        Err(AgentError::ReplicaError(replica_error)) => {
+        Err(AgentError::UncertifiedReject(replica_error)) => {
             assert_eq!(replica_error.reject_code, RejectCode::DestinationInvalid);
             assert_eq!(replica_error.reject_message, "Rejected Message");
             assert_eq!(replica_error.error_code, Some("Error code".to_string()));
@@ -202,7 +202,7 @@ async fn call_rejected() -> Result<(), AgentError> {
 
     assert_mock(call_mock).await;
 
-    let expected_response = Err(AgentError::ReplicaError(reject_body));
+    let expected_response = Err(AgentError::UncertifiedReject(reject_body));
     assert_eq!(expected_response, result);
 
     Ok(())
@@ -238,7 +238,7 @@ async fn call_rejected_without_error_code() -> Result<(), AgentError> {
 
     assert_mock(call_mock).await;
 
-    let expected_response = Err(AgentError::ReplicaError(reject_body));
+    let expected_response = Err(AgentError::UncertifiedReject(reject_body));
     assert_eq!(expected_response, result);
 
     Ok(())

diff --git a/ic-agent/src/agent/http_transport/reqwest_transport.rs b/ic-agent/src/agent/http_transport/reqwest_transport.rs
@@ -146,7 +146,7 @@ impl ReqwestTransport {
                 serde_cbor::from_slice(&body);
 
             let agent_error = match cbor_decoded_body {
-                Ok(replica_error) => AgentError::ReplicaError(replica_error),
+                Ok(replica_error) => AgentError::UncertifiedReject(replica_error),
                 Err(cbor_error) => AgentError::InvalidCborData(cbor_error),
             };
 

diff --git a/ic-agent/src/agent/mod.rs b/ic-agent/src/agent/mod.rs
@@ -550,7 +550,7 @@ impl Agent {
 
         match response {
             QueryResponse::Replied { reply, .. } => Ok(reply.arg),
-            QueryResponse::Rejected { reject, .. } => Err(AgentError::ReplicaError(reject)),
+            QueryResponse::Rejected { reject, .. } => Err(AgentError::UncertifiedReject(reject)),
         }
     }
 
@@ -650,7 +650,7 @@ impl Agent {
                 Ok(PollResult::Completed(arg))
             }
 
-            RequestStatusResponse::Rejected(response) => Err(AgentError::ReplicaError(response)),
+            RequestStatusResponse::Rejected(response) => Err(AgentError::CertifiedReject(response)),
 
             RequestStatusResponse::Done => Err(AgentError::RequestStatusDoneNoReply(String::from(
                 *request_id,
@@ -698,7 +698,7 @@ impl Agent {
                 RequestStatusResponse::Replied(ReplyResponse { arg, .. }) => return Ok(arg),
 
                 RequestStatusResponse::Rejected(response) => {
-                    return Err(AgentError::ReplicaError(response))
+                    return Err(AgentError::CertifiedReject(response))
                 }
 
                 RequestStatusResponse::Done => {

diff --git a/ic-utils/src/interfaces/wallet.rs b/ic-utils/src/interfaces/wallet.rs
@@ -436,7 +436,7 @@ impl<'agent> WalletCanister<'agent> {
         let version: Result<(String,), _> =
             canister.query("wallet_api_version").build().call().await;
         let version = match version {
-            Err(AgentError::ReplicaError(replica_error))
+            Err(AgentError::UncertifiedReject(replica_error))
                 if replica_error.reject_code == RejectCode::DestinationInvalid
                     && (replica_error
                         .reject_message

diff --git a/ref-tests/tests/ic-ref.rs b/ref-tests/tests/ic-ref.rs
@@ -96,7 +96,7 @@ mod management_canister {
                     .await;
 
                 assert!(matches!(result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                     reject_code: RejectCode::DestinationInvalid,
                     reject_message,
                     error_code: Some(ref error_code)
@@ -136,7 +136,7 @@ mod management_canister {
                 .call_and_wait()
                 .await;
 
-            assert!(matches!(result, Err(AgentError::ReplicaError { .. })));
+            assert!(matches!(result, Err(AgentError::CertifiedReject { .. })));
 
             // Reinstall should succeed.
             ic00.install_code(&canister_id, &canister_wasm)
@@ -157,7 +157,7 @@ mod management_canister {
                 .with_mode(InstallMode::Reinstall)
                 .call_and_wait()
                 .await;
-            assert!(matches!(result, Err(AgentError::ReplicaError(..))));
+            assert!(matches!(result, Err(AgentError::UncertifiedReject(..))));
 
             // Upgrade should succeed.
             ic00.install_code(&canister_id, &canister_wasm)
@@ -175,7 +175,7 @@ mod management_canister {
                 })
                 .call_and_wait()
                 .await;
-            assert!(matches!(result, Err(AgentError::ReplicaError(..))));
+            assert!(matches!(result, Err(AgentError::UncertifiedReject(..))));
 
             // Change controller.
             ic00.update_settings(&canister_id)
@@ -190,7 +190,7 @@ mod management_canister {
                 .call_and_wait()
                 .await;
             assert!(
-                matches!(result, Err(AgentError::ReplicaError(RejectResponse{
+                matches!(result, Err(AgentError::UncertifiedReject(RejectResponse{
                 reject_code: RejectCode::CanisterError,
                 reject_message,
                 error_code: Some(ref error_code),
@@ -404,7 +404,7 @@ mod management_canister {
     ) {
         for expected_rc in &allowed_reject_codes {
             if matches!(result,
-                Err(AgentError::ReplicaError(RejectResponse {
+                Err(AgentError::UncertifiedReject(RejectResponse {
                 reject_code,
                 ..
             })) if reject_code == *expected_rc)
@@ -415,7 +415,7 @@ mod management_canister {
 
         assert!(
             matches!(result, Err(AgentError::HttpError(_))),
-            "expect an HttpError, or a ReplicaError with reject_code in {:?}",
+            "expect an HttpError, or a CertifiedReject with reject_code in {:?}",
             allowed_reject_codes
         );
     }
@@ -458,7 +458,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -473,7 +473,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -503,7 +503,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::CertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: None,
@@ -517,7 +517,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: Some(error_code),
@@ -541,7 +541,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: Some(error_code),
@@ -556,7 +556,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: Some(error_code),
@@ -570,7 +570,7 @@ mod management_canister {
             let result = ic00.canister_status(&canister_id).call_and_wait().await;
             assert!(
                 match &result {
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: Some(error_code),
@@ -590,7 +590,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse{
+                    Err(AgentError::UncertifiedReject(RejectResponse{
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: Some(error_code),
@@ -636,7 +636,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -651,7 +651,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -669,7 +669,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -687,7 +687,7 @@ mod management_canister {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::CanisterError,
                         reject_message,
                         error_code: Some(error_code),
@@ -959,7 +959,7 @@ mod simple_calls {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::CertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         ..
                     })),
@@ -984,7 +984,7 @@ mod simple_calls {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::UncertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         ..
                     }))
@@ -1218,7 +1218,7 @@ mod extras {
             assert!(
                 matches!(
                     &result,
-                    Err(AgentError::ReplicaError(RejectResponse {
+                    Err(AgentError::CertifiedReject(RejectResponse {
                         reject_code: RejectCode::DestinationInvalid,
                         reject_message,
                         error_code: None,

diff --git a/ref-tests/tests/integration.rs b/ref-tests/tests/integration.rs
@@ -187,7 +187,7 @@ fn canister_reject_call() {
 
         assert!(matches!(
             result,
-            Err(AgentError::ReplicaError(RejectResponse {
+            Err(AgentError::CertifiedReject(RejectResponse {
                 reject_code: RejectCode::DestinationInvalid,
                 reject_message,
                 error_code: None,