update contexts in which ic0.msg_caller_size and ic0.msg_caller_copy can be called

mraszyk commented 1 year ago

It does not make sense to prevent retrieving the caller in response (reply and reject) and cleanup callbacks so this PR makes this possible. In particular, calling ic_cdk::caller() after an await point should not trap in an update method. Moreover, this PR makes ic_principal (the management canister ID) the caller of system tasks and their callbacks.

netlify[bot] commented 1 year ago

Deploy Preview for ic-interface-spec ready!

Name	Link
Latest commit	0a95363656c4a106566842abd39eedff39f023cd
Latest deploy log	https://app.netlify.com/sites/ic-interface-spec/deploys/64c3c06a14f7e6000834b602
Deploy Preview	https://deploy-preview-201--ic-interface-spec.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dsarlis commented 1 year ago

Reading this I'm left with a question: in case a call is made from a heartbeat/timer (i.e. a system task), there's no caller and indeed for context T we don't allow to call ic_cdk::caller(). However, if we're in a callback triggered by a system task, at least reading the spec, I might get the impression that the caller can be retrieved when in reality there isn't any caller. Can we clarify this point somehow?

Edit: I see we have now specific callback contexts for composite queries so perhaps the easiest is to introduce specific callback contexts for system tasks?

mraszyk commented 1 year ago

I see we have now specific callback contexts for composite queries so perhaps the easiest is to introduce specific callback contexts for system tasks?

That would indeed be an option, but I see an even easier way around the issue:

allow calling ic0.msg_caller_size and ic0.msg_caller_copy in system tasks T as well as response callbacks Ry and Rt and cleanup callback C;
define the caller as the management canister (aaaaa-aa), i.e., the system, for system tasks, both in their entry point (context T), response callbacks (contexts Ry and Rt), and cleanup callbacks (context C).

It seems to me that this would might also simplify the replica code change as no optional caller type are necessary, although some additional test(s) might be necessary.

@dsarlis What do you think about my suggestion?

dsarlis commented 1 year ago

I see we have now specific callback contexts for composite queries so perhaps the easiest is to introduce specific callback contexts for system tasks?

That would indeed be an option, but I see an even easier way around the issue:

allow calling ic0.msg_caller_size and ic0.msg_caller_copy in system tasks T as well as response callbacks Ry and Rt and cleanup callback C;

define the caller as the management canister (aaaaa-aa), i.e., the system, for system tasks, both in their entry point (context T), response callbacks (contexts Ry and Rt), and cleanup callbacks (context C).

It seems to me that this would might also simplify the replica code change as no optional caller type are necessary, although some additional test(s) might be necessary.

@dsarlis What do you think about my suggestion?

I'm not sure tbh. On one hand, it does seem like an option to have the management canister be the caller of these tasks, but on the other, conceptually it doesn't seem right as there is no call that has been made in that case, nor any message in the canister's input queue.

The replica change I think is simple enough even with the optional caller for reply/reject callbacks and if we changed it to your suggestion, it feels like we'd be trying to make things more consistent by adding a somewhat questionable semantic (a "caller" for system triggered tasks).

This is my personal opinion, I don't feel super strongly, so if there's consensus from the spec front to make this change, I'm fine with it.

Dfinity-Bjoern commented 1 year ago

I have a slight preference for using aaaaa-aa, because of the following: You could be using the same function as a callback from both U and T contexts, and if calling a system API in one of the two cases traps and in the other it doesn't, it's practically not usable. There's of course another way of resolving this: making the call context explicit to the canister through an additional API.

dsarlis commented 1 year ago

I have a slight preference for using aaaaa-aa, because of the following: You could be using the same function as a callback from both U and T contexts, and if calling a system API in one of the two cases traps and in the other it doesn't, it's practically not usable.

I'm ok having aaaaa-aa as the caller, the use case you brought up is a good one. We can make the change.

There's of course another way of resolving this: making the call context explicit to the canister through an additional API.

The call context maintained by the system is relatively complex data structure, I'm not sure I'd go down that path because it's gonna require some thinking how we can encode the different aspects. The caller seems to be the most useful part for canisters anyway.

dfinity / interface-spec