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

feat!: Handle rejected update calls after IC spec change. #422

Merged
merged 29 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8886551
Returning AgentError:ReplicaError for 200 status codes
DSharifi Apr 13, 2023
53f067c
Added condition that endpoint is call
DSharifi Apr 13, 2023
9ea71b8
Added ReplicaError struct to enum variants
DSharifi Apr 19, 2023
788180d
Renamed inner struct type to RejectedResponse
DSharifi Apr 19, 2023
f7ae406
Changed error message for ReplicaError
DSharifi Apr 19, 2023
c476d27
Removed dead commented code
DSharifi Apr 19, 2023
6cf3850
Added integration test for when error message is in body
DSharifi Apr 20, 2023
7f8505a
Refactored integration test for call rejected
DSharifi Apr 20, 2023
705933d
Refactored pattern matching in tests
DSharifi Apr 20, 2023
768c671
Revert "Refactored pattern matching in tests"
DSharifi Apr 20, 2023
cb23a57
Updated changelog
DSharifi Apr 20, 2023
29c0190
Fixed clippy lint errors
DSharifi Apr 20, 2023
8e7bacf
Improved comment
DSharifi Apr 20, 2023
b1f207f
Merged imports
DSharifi Apr 20, 2023
b498564
Added error code to RejecResponse::Display
DSharifi Apr 20, 2023
b44042a
Cahgned RejectResponse do to IC execution error
DSharifi Apr 20, 2023
1d9c84b
Added u8 representation for RejectCode
DSharifi Apr 20, 2023
5ce6775
Added serialize/deserialize repr for Rejectcode
DSharifi Apr 20, 2023
80cffed
Added println for debugging
DSharifi Apr 20, 2023
a49bc72
Added error code check to integration test
DSharifi Apr 20, 2023
c20a1f9
Fixed typo in CHANGELOG.md
DSharifi Apr 20, 2023
00243e3
Merged imports from replica_api
DSharifi Apr 20, 2023
8773a0c
Merge branch 'handle-error-messages-in-http-body' of github.com:DShar…
DSharifi Apr 20, 2023
21c061e
Fixed typo
DSharifi Apr 20, 2023
b112d48
Disregarding error_code for pattern matching in ic-ref tests
DSharifi Apr 20, 2023
d9ed70e
Added error code to query reject test
DSharifi Apr 21, 2023
079231a
Added error_code to error messge
DSharifi Apr 21, 2023
3d3caae
Merge branch 'main' into handle-error-messages-in-http-body
DSharifi Apr 21, 2023
bdc1cea
Merge branch 'main' into handle-error-messages-in-http-body
ericswanson-dfinity Apr 21, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Unreleased
* Breaking Change: Enum variant `AgentError::ReplicaError` is now a tuple struct containing `RejectResponse`.
* Handling rejected update calls where status code is 200. See IC-1462
* Reject code type is changed from `u64` to enum `RejectCode`.

* Support WASM targets in the browser via `wasm-bindgen`

Expand Down
12 changes: 12 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 ic-agent/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ ring = { workspace = true, features = ["std"] }
serde = { workspace = true, features = ["derive"] }
serde_bytes = { workspace = true }
serde_cbor = "0.11.2"
serde_repr = "0.1.12"
sha2 = { workspace = true }
simple_asn1 = "0.6.1"
thiserror = { workspace = true }
Expand Down
24 changes: 16 additions & 8 deletions ic-agent/src/agent/agent_error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
//! Errors that can occur when using the replica agent.

use crate::{agent::status::Status, RequestIdError};
use crate::{
agent::{replica_api::RejectResponse, status::Status},
RequestIdError,
};
// use crate::{agent::status::Status, RequestIdError};
DSharifi marked this conversation as resolved.
Show resolved Hide resolved
use ic_certification::Label;
use leb128::read;
use std::{
Expand Down Expand Up @@ -49,13 +53,8 @@ pub enum AgentError {
PrincipalError(#[from] crate::export::PrincipalError),

/// The replica rejected the message.
#[error(r#"The Replica returned an error: code {reject_code}, message: "{reject_message}""#)]
ReplicaError {
/// The [reject code](https://smartcontracts.org/docs/interface-spec/index.html#reject-codes) returned by the replica.
reject_code: u64,
/// The rejection message.
reject_message: String,
},
#[error("The replica returned a replica error: {0}")]
ReplicaError(RejectResponse),

/// The replica returned an HTTP error.
#[error("The replica returned an HTTP Error: {0}")]
Expand Down Expand Up @@ -193,6 +192,15 @@ impl PartialEq for AgentError {
}
}

impl Display for RejectResponse {
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> {
f.write_fmt(format_args!(
"Replica Error: reject code {:?}, reject message {}, error code {:?}",
self.reject_code, self.reject_message, self.error_code,
))
}
}

/// A HTTP error from the replica.
pub struct HttpErrorPayload {
/// The HTTP status code.
Expand Down
95 changes: 85 additions & 10 deletions ic-agent/src/agent/agent_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use self::mock::{assert_mock, mock};
use crate::{
agent::{
http_transport::ReqwestTransport,
replica_api::{CallReply, QueryResponse},
replica_api::{CallReply, QueryResponse, RejectCode, RejectResponse},
Status,
},
export::Principal,
Expand Down Expand Up @@ -85,10 +85,11 @@ async fn query_error() -> Result<(), AgentError> {
#[cfg_attr(not(target_family = "wasm"), tokio::test)]
#[cfg_attr(target_family = "wasm", wasm_bindgen_test)]
async fn query_rejected() -> Result<(), AgentError> {
DSharifi marked this conversation as resolved.
Show resolved Hide resolved
let response: QueryResponse = QueryResponse::Rejected {
reject_code: 1234,
let response: QueryResponse = QueryResponse::Rejected(RejectResponse {
reject_code: RejectCode::DestinationInvalid,
reject_message: "Rejected Message".to_string(),
};
error_code: Some("Error code".to_string()),
});

let (query_mock, url) = mock(
"POST",
Expand Down Expand Up @@ -116,12 +117,10 @@ async fn query_rejected() -> Result<(), AgentError> {
assert_mock(query_mock).await;

match result {
Err(AgentError::ReplicaError {
reject_code: code,
reject_message: msg,
}) => {
assert_eq!(code, 1234);
assert_eq!(msg, "Rejected Message");
Err(AgentError::ReplicaError(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()));
}
result => unreachable!("{:?}", result),
}
Expand Down Expand Up @@ -151,6 +150,82 @@ async fn call_error() -> Result<(), AgentError> {
Ok(())
}

#[cfg_attr(not(target_family = "wasm"), tokio::test)]
#[cfg_attr(target_family = "wasm", wasm_bindgen_test)]
async fn call_rejected() -> Result<(), AgentError> {
let reject_body = RejectResponse {
reject_code: RejectCode::SysTransient,
reject_message: "Test reject message".to_string(),
error_code: Some("Test error code".to_string()),
};

let body = serde_cbor::to_vec(&reject_body).unwrap();

let (call_mock, url) = mock(
"POST",
"/api/v2/canister/aaaaa-aa/call",
200,
body,
Some("application/cbor"),
)
.await;

let agent = Agent::builder()
.with_transport(ReqwestTransport::create(&url)?)
.build()?;

let result = agent
.update(&Principal::management_canister(), "greet")
.with_arg([])
.call()
.await;

assert_mock(call_mock).await;

let expected_response = Err(AgentError::ReplicaError(reject_body));
assert_eq!(expected_response, result);

Ok(())
}

#[cfg_attr(not(target_family = "wasm"), tokio::test)]
#[cfg_attr(target_family = "wasm", wasm_bindgen_test)]
async fn call_rejected_without_error_code() -> Result<(), AgentError> {
let reject_body = RejectResponse {
reject_code: RejectCode::SysTransient,
reject_message: "Test reject message".to_string(),
error_code: None,
};

let body = serde_cbor::to_vec(&reject_body).unwrap();

let (call_mock, url) = mock(
"POST",
"/api/v2/canister/aaaaa-aa/call",
200,
body,
Some("application/cbor"),
)
.await;

let agent = Agent::builder()
.with_transport(ReqwestTransport::create(&url)?)
.build()?;

let result = agent
.update(&Principal::management_canister(), "greet")
.with_arg([])
.call()
.await;

assert_mock(call_mock).await;

let expected_response = Err(AgentError::ReplicaError(reject_body));
assert_eq!(expected_response, result);

Ok(())
}

#[cfg_attr(not(target_family = "wasm"), tokio::test)]
#[cfg_attr(target_family = "wasm", wasm_bindgen_test)]
async fn status() -> Result<(), AgentError> {
Expand Down
15 changes: 14 additions & 1 deletion ic-agent/src/agent/http_transport/reqwest_transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use crate::{
agent::{
agent_error::HttpErrorPayload,
http_transport::{IC0_DOMAIN, IC0_SUB_DOMAIN},
replica_api::RejectResponse,
AgentFuture, Transport,
},
export::Principal,
Expand Down Expand Up @@ -236,7 +237,19 @@ impl ReqwestTransport {
}
}

if status.is_client_error() || status.is_server_error() {
// status == OK means we have an error message for call requests
// see https://internetcomputer.org/docs/current/references/ic-interface-spec#http-call
if status == StatusCode::OK && endpoint.ends_with("call") {
DSharifi marked this conversation as resolved.
Show resolved Hide resolved
let cbor_decoded_body: Result<RejectResponse, serde_cbor::Error> =
serde_cbor::from_slice(&body);

let agent_error = match cbor_decoded_body {
Ok(replica_error) => AgentError::ReplicaError(replica_error),
Err(cbor_error) => AgentError::InvalidCborData(cbor_error),
};

Err(agent_error)
} else if status.is_client_error() || status.is_server_error() {
Err(AgentError::HttpError(HttpErrorPayload {
status: status.into(),
content_type: headers
Expand Down
30 changes: 9 additions & 21 deletions ic-agent/src/agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub use agent_error::AgentError;
use backoff::{backoff::Backoff, ExponentialBackoffBuilder};
pub use builder::AgentBuilder;
pub use nonce::{NonceFactory, NonceGenerator};
pub use replica_api::{RejectCode, RejectResponse};
pub use response::{Replied, RequestStatusResponse};

#[cfg(test)]
Expand Down Expand Up @@ -421,13 +422,9 @@ impl Agent {
.await
.and_then(|response| match response {
replica_api::QueryResponse::Replied { reply } => Ok(reply.arg),
replica_api::QueryResponse::Rejected {
reject_code,
reject_message,
} => Err(AgentError::ReplicaError {
reject_code,
reject_message,
}),
replica_api::QueryResponse::Rejected(response) => {
DSharifi marked this conversation as resolved.
Show resolved Hide resolved
Err(AgentError::ReplicaError(response))
}
})
}

Expand All @@ -445,13 +442,9 @@ impl Agent {
.await
.and_then(|response| match response {
replica_api::QueryResponse::Replied { reply } => Ok(reply.arg),
replica_api::QueryResponse::Rejected {
reject_code,
reject_message,
} => Err(AgentError::ReplicaError {
reject_code,
reject_message,
}),
replica_api::QueryResponse::Rejected(response) => {
Err(AgentError::ReplicaError(response))
}
})
}

Expand Down Expand Up @@ -542,13 +535,8 @@ impl Agent {
reply: Replied::CallReplied(arg),
} => Ok(PollResult::Completed(arg)),

RequestStatusResponse::Rejected {
reject_code,
reject_message,
} => Err(AgentError::ReplicaError {
reject_code,
reject_message,
}),
RequestStatusResponse::Rejected(response) => Err(AgentError::ReplicaError(response)),

RequestStatusResponse::Done => Err(AgentError::RequestStatusDoneNoReply(String::from(
*request_id,
))),
Expand Down
56 changes: 51 additions & 5 deletions ic-agent/src/agent/replica_api.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::export::Principal;
use crate::{export::Principal, AgentError};
use ic_certification::Label;
use serde::{Deserialize, Serialize};

pub use ic_certification::{Certificate, Delegation};
use serde_repr::{Deserialize_repr, Serialize_repr};

#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "snake_case")]
Expand Down Expand Up @@ -146,8 +147,53 @@ pub enum QueryResponse {
#[serde(rename = "replied")]
Replied { reply: CallReply },
#[serde(rename = "rejected")]
Rejected {
reject_code: u64,
reject_message: String,
},
Rejected(RejectResponse),
}

/// An IC execution error received from the replica.
#[derive(Debug, Clone, Serialize, Deserialize, Ord, PartialOrd, Eq, PartialEq)]
pub struct RejectResponse {
/// The [reject code](https://smartcontracts.org/docs/interface-spec/index.html#reject-codes) returned by the replica.
pub reject_code: RejectCode,
/// The rejection message.
pub reject_message: String,
/// The optional [error code](https://smartcontracts.org/docs/interface-spec/index.html#error-codes) returned by the replica.
#[serde(default)]
pub error_code: Option<String>,
}

/// See the [interface spec](https://smartcontracts.org/docs/interface-spec/index.html#reject-codes).
#[derive(
Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize_repr, Deserialize_repr, Ord, PartialOrd,
)]
#[repr(u8)]
pub enum RejectCode {
/// Fatal system error, retry unlikely to be useful
SysFatal = 1,
/// Transient system error, retry might be possible.
SysTransient = 2,
/// Invalid destination (e.g. canister/account does not exist)
DestinationInvalid = 3,
/// Explicit reject by the canister.
CanisterReject = 4,
/// Canister error (e.g., trap, no response)
CanisterError = 5,
}

impl TryFrom<u64> for RejectCode {
type Error = AgentError;

fn try_from(value: u64) -> Result<Self, AgentError> {
match value {
1 => Ok(RejectCode::SysFatal),
2 => Ok(RejectCode::SysTransient),
3 => Ok(RejectCode::DestinationInvalid),
4 => Ok(RejectCode::CanisterReject),
5 => Ok(RejectCode::CanisterError),
_ => Err(AgentError::MessageError(format!(
DSharifi marked this conversation as resolved.
Show resolved Hide resolved
"Received an invalid reject code {}",
value
))),
}
}
}
9 changes: 3 additions & 6 deletions ic-agent/src/agent/response.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use super::replica_api::RejectResponse;
DSharifi marked this conversation as resolved.
Show resolved Hide resolved

/// The response of /api/v2/canister/<effective_canister_id>/read_state with "request_status" request type.
///
/// See [the HTTP interface specification](https://smartcontracts.org/docs/interface-spec/index.html#http-call-overview) for more details.
Expand All @@ -15,12 +17,7 @@ pub enum RequestStatusResponse {
reply: Replied,
},
/// The request has been rejected.
Rejected {
/// The [reject code](https://smartcontracts.org/docs/interface-spec/index.html#reject-codes) from the replica.
reject_code: u64,
/// The rejection message.
reject_message: String,
},
Rejected(RejectResponse),
/// The call has been completed, and it has been long enough that the reply/reject data has been purged, but the call has not expired yet.
Done,
}
Expand Down
Loading