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.

Sign up for GitHub

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!: change ReplicaError to Certified/UncertifiedReject #531

Merged
merged 5 commits into from
Mar 13, 2024
Merged

feat!: change ReplicaError to Certified/UncertifiedReject #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bffaaad
breaking: change ReplicaError to Certified/UncertifiedReject
adamspofford-dfinity Mar 12, 2024
80644ed
Update changelog
adamspofford-dfinity Mar 12, 2024
3895cd8
Merge branch 'main' into spofford/uncertified-errors
adamspofford-dfinity Mar 13, 2024
8fffc73
Merge branch 'main' into spofford/uncertified-errors
adamspofford-dfinity Mar 13, 2024
1c70ee0
Describe code change
adamspofford-dfinity Mar 13, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 7 additions & 3 deletions ic-agent/src/agent/agent_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand Down
6 changes: 3 additions & 3 deletions ic-agent/src/agent/agent_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion ic-agent/src/agent/http_transport/reqwest_transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};

Expand Down
6 changes: 3 additions & 3 deletions ic-agent/src/agent/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
}
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 => {
Expand Down
2 changes: 1 addition & 1 deletion ic-utils/src/interfaces/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
44 changes: 22 additions & 22 deletions ref-tests/tests/ic-ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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),
Expand Down Expand Up @@ -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)
Expand All @@ -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
);
}
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -959,7 +959,7 @@ mod simple_calls {
assert!(
matches!(
&result,
Err(AgentError::ReplicaError(RejectResponse {
Err(AgentError::CertifiedReject(RejectResponse {
reject_code: RejectCode::DestinationInvalid,
..
})),
Expand All @@ -984,7 +984,7 @@ mod simple_calls {
assert!(
matches!(
&result,
Err(AgentError::ReplicaError(RejectResponse {
Err(AgentError::UncertifiedReject(RejectResponse {
reject_code: RejectCode::DestinationInvalid,
..
}))
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion ref-tests/tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down