feat: CallError redesign #548

lwshang · 2025-01-16T15:25:53Z

Description

There are a few reason we need a redesign of the CallError:

The previous design made it hard to get the RejectCode which is the most common used field to do error handling.
There is an initiative to expose error_code in system API. To avoid breaking changes at that time, we start to include error_code now.

Error types

Aside from the CallError, I introduced SystemError. SystemError is a variant of CallError.

The error type of call_raw() and call_oneway() is SystemError. It implies that the call is rejected by the system.

pub struct SystemError {
    pub reject_code: RejectCode,
    pub reject_message: String,
    pub error_code: ErrorCode,
    pub sync: bool,
}

The error type of call() and call_tuple is CallError. They might also failed when decoding the reply.

pub enum CallError {
    CallRejected(SystemError),
    CandidDecodeFailed(String),
}

Error codes

Borrowed the ErrorCode implementation from pocket-ic.

How Has This Been Tested?

ic-cdk-timers internally make inter-canister calls to itself. The change there demonstrates that the error handling is simplified.

Checklist:

The title of this PR complies with Conventional Commits.
I have edited the CHANGELOG accordingly.
I have made corresponding changes to the documentation.

ic-cdk-timers/src/lib.rs

ericswanson-dfinity · 2025-01-16T17:45:18Z

ic-cdk/src/call.rs

+    /// As of the current version of the IC, the error codes are not available in the system API.
+    /// Please DO NOT rely on the error codes until they are officially supported.
+    pub error_code: ErrorCode,


What's the purpose of having this 'pub' when the value is generally incorrect (from reject_to_error) and the error codes haven't been officially finalized?

We can remove the pub for now. But when ic0.msg_error_code is officially supported, we will need to add back the pub. Then that will be a breaking change.

One alternative is to make the fields private and provide public getters. But that will harm the UX when users want to extract multiple fields.

After more thinking, I feel that the alternative approach above could be better. I will followup with Martin.

ic-cdk/src/call.rs

Co-authored-by: Eric Swanson <[email protected]>

ic-cdk-timers/src/lib.rs

mraszyk · 2025-01-17T07:12:20Z

ic-cdk/src/call.rs

-            // 0 is a special code meaning "no error"
-            0 => RejectCode::NoError,


would it make sense to assert that the code is not zero?

There is no need of assertion right? Since there is no variant for 0 now. And the RejectCode instances are constructed using TryFrom<u32> which will panic for 0.

And the RejectCode instances are constructed using TryFrom which will panic for 0.

I missed that panic which is actually dangerous - please see my other comment. Panicking in case of 0 makes sense though since 0 can never become a new reject code.

mraszyk · 2025-01-17T07:15:56Z

ic-cdk/src/call.rs

+    /// Returns the reject message.
+    ///
+    /// When the call was rejected asynchronously (IC rejects the call after it was enqueued),
+    /// this message is set with [`msg_reject`](crate::api::msg_reject).


the reject message can also be produced by the system (canister stopped, out of cycles etc.) or come from crate::api::trap (including a wrapper by the system in case of traps)

That's a typo.
I wanted to mention

[`msg_reject_msg`](crate::api::msg_reject_msg).

mraszyk · 2025-01-17T07:17:04Z

ic-cdk/src/call.rs

+    /// this message is set with [`msg_reject`](crate::api::msg_reject).
+    ///
+    /// When the call was rejected synchronously (`ic0.call_preform` returns non-zero code),
+    /// this message is set to a fixed string ("failed to enqueue the call").


I'd recommend to stick to the current message:

for the sake of backward-compatibility (no change in behavior)

"failed to enqueue the call" sounds like the queue is the issue, but the failure could also be due to cycles (for instance)

mraszyk · 2025-01-17T07:18:30Z

ic-cdk/src/call.rs

-    async fn call_raw(self) -> CallResult<Vec<u8>> {
-        let args_raw = encode_args(self.args).map_err(encoder_error_to_call_error::<T>)?;
+    async fn call_raw(self) -> SystemResult<Vec<u8>> {
+        // Candid Encoding can only fail if heap memory is exhausted.


if the heap memory is exhausted, the canister traps anyway

mraszyk · 2025-01-17T07:19:49Z

ic-cdk/src/call.rs

+                        let reject_code = RejectCode::try_from(code).unwrap();
+                        let result = Err(CallRejected {
+                            reject_code,
+                            reject_message: "failed to enqueue the call".to_string(),


Could we define this as a string constant given that it is used at multiple places?

mraszyk · 2025-01-17T15:37:06Z

ic-cdk/src/call.rs

+            state.write().unwrap().result = Some(match msg_reject_code() {
+                0 => Ok(msg_arg_data()),
+                code => {
+                    let reject_code = RejectCode::try_from(code).unwrap();


this unwrap could be dangerous for canisters with an outdated CDK version if the replica introduces a new reject code

lwshang added 9 commits January 15, 2025 20:09

Remove CandidEncodeFailed variant

70d1c1a

Unify RejectCode

fedcf2c

RejectCode doesn't include 0

7b88edc

CallPerform also has a hard-coded reject message

a8a87ac

SystemError v.s. CallError

a3ea5c2

avoid candid en/decoding in timers

556a19e

SystemError includes error_code

71c27c8

Clarify 0 timeout for change_timeout()

c10db2c

fix clippy

a481e62

lwshang marked this pull request as ready for review January 16, 2025 15:37

lwshang requested a review from a team as a code owner January 16, 2025 15:37

lwshang requested a review from mraszyk January 16, 2025 15:40

ericswanson-dfinity reviewed Jan 16, 2025

View reviewed changes

ic-cdk-timers/src/lib.rs Outdated Show resolved Hide resolved

simplify retry_later logic

4d828c3

lwshang requested a review from ericswanson-dfinity January 16, 2025 17:09

ericswanson-dfinity reviewed Jan 16, 2025

View reviewed changes

rename SystemError to CallRejected and make fields private

47caeb0

ericswanson-dfinity reviewed Jan 16, 2025

View reviewed changes

ic-cdk/src/call.rs Outdated Show resolved Hide resolved

fix typo

e25bcf0

Co-authored-by: Eric Swanson <[email protected]>

ericswanson-dfinity approved these changes Jan 16, 2025

View reviewed changes

lwshang merged commit 68f98ce into next Jan 16, 2025
11 checks passed

lwshang deleted the lwshang/CallError branch January 16, 2025 19:15

mraszyk reviewed Jan 17, 2025

View reviewed changes

ic-cdk-timers/src/lib.rs Show resolved Hide resolved

mraszyk reviewed Jan 17, 2025

View reviewed changes

lwshang mentioned this pull request Jan 17, 2025

chore: call module followup #549

Merged

alin-at-dfinity mentioned this pull request Jan 20, 2025

test: Add best effort messages to random traffic canister dfinity/ic#3108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CallError redesign #548

feat: CallError redesign #548

lwshang commented Jan 16, 2025 •

edited

Loading

ericswanson-dfinity Jan 16, 2025

lwshang Jan 16, 2025

mraszyk Jan 17, 2025

lwshang Jan 17, 2025

mraszyk Jan 17, 2025

mraszyk Jan 17, 2025

lwshang Jan 17, 2025

mraszyk Jan 17, 2025

mraszyk Jan 17, 2025

mraszyk Jan 17, 2025

mraszyk Jan 17, 2025

		// 0 is a special code meaning "no error"
		0 => RejectCode::NoError,

feat: CallError redesign #548

feat: CallError redesign #548

Conversation

lwshang commented Jan 16, 2025 • edited Loading

Description

Error types

Error codes

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwshang commented Jan 16, 2025 •

edited

Loading