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

Add "SubCall" to get result from a message #691

Closed
ethanfrey opened this issue Jan 5, 2021 · 7 comments · Fixed by #796
Closed

Add "SubCall" to get result from a message #691

ethanfrey opened this issue Jan 5, 2021 · 7 comments · Fixed by #796
Milestone

Comments

@ethanfrey
Copy link
Member

ethanfrey commented Jan 5, 2021

Closes #292 (concrete solution to that issue)

We can add a new variant to CosmosMsg, like:

pub enum CosmosMsg<T> {
  // ...
  SubCall{
      id: String,
      msg: Box<CosmosMsg<T>>,
      gas_limit: Option<u64>,
  }
}

CosmosMsg::SubCall would have the following semantics:

  • SubCall.msg would be executed in a sub-transaction, with an optional gas limit (so it cannot use up all the gas of the caller)
    • If the message fails, all state changes from executing SubCall.msg would be reverted and an error returned to the caller (see below)
    • If the message succeeds, the state changes from executing SubCall.msg would be committed and the result returned to the caller
    • SubCall.msg == CosmosMsg::SubCall is illegal and returns an error (aborting the transaction, not returned)
  • After SubCall.msg is executed, if the transaction has not aborted (illegal values in CosmosMsg::SubCall or out of gas), then the calling contract is guaranteed to get a callback in the same atomic transaction.
    • The callback contains the id, as well as the original message and the result or error.
    • We define a new entry point for this callback, which can be wrapped like handle, but exposed like this in a contract
// no MessageInfo needed, as this is a callback from ourselves
pub fn callback(
    deps: DepsMut,
    env: Env,
    // this is the id we passed in the original call, so we can look this up
    id: String,
    // this is the result from the call. If it returned an error, we just get a stringified version 
    result: Result<SubCallResponse, String>
) -> Result<CallbackResponse, ContractError> {
 // ...
}

/// The information we get back from the sub-call, with full sdk events
pub struct CallbackResponse {
    pub events: Vec<Event>,
    pub data: Option<Binary>,
}

pub struct Event {
    pub type: String,
    pub attributes: Vec<Attribute>,
}

/// Same as HandleResponse
pub struct CallbackResponse<T = Empty>
where
    T: Clone + fmt::Debug + PartialEq + JsonSchema,
{
    pub messages: Vec<CosmosMsg<T>>,
    /// The attributes that will be emitted as part of a "wasm" event
    pub attributes: Vec<Attribute>,
    pub data: Option<Binary>,
}

To keep all the guarantees the much of the cosmos sdk modules depend on, we need to revert any partial state changes from the subcall if it errors.

By returning the result and not aborting, we allow the caller to handle it. Maybe a module triggers an optional listener with a gas limit. If it fails, we can record it as failed and keep processing the main work. By returning a Result type here, we make it easy to simply "abort on error in subcall" with a line or two, but allow the caller to also handle that case if they wish.

Note the callback entry point is not required on modules. However, if they return CosmosMsg::SubCall and do not export that entry point, this will abort the transaction.

@ethanfrey
Copy link
Member Author

Any feedback on this design?
I would be happy to implement next week, but I would like a design review and any thoughts/objections before I start coding.

@alpe @webmaster128 I discussed this idea with you before. Input on how to improve the proposal?

@webmaster128
Copy link
Member

webmaster128 commented Feb 8, 2021

I like the flow, makes much sense.

What about avoiding the term "call"/"callback" alltogether to avoid any kind of expectation for Ethereum-like behaviour. What this really is is a message that is emitted one way for which the contract expects a "reply".

I would avoid the introduction of fake message types to keep a 1:1 mapping to between CosmosMsg and Cosmos SDK messages. But I also think this works fundamentally different than the HandleResponse::messages, which are emitted and never returned. So what about moving those to a different level:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct HandleResponse<T = Empty>
where
    T: Clone + fmt::Debug + PartialEq + JsonSchema,
{
    /// Sub-messages trigger executions that share the same gas budget with the original contract execution.
    /// They are processed before all regular messages. Each sub-message causes a reply to be sent to
    /// this contract via the `reply` entry point.
    pub sub_messages: Vec<SubMsg<T>>,
    pub messages: Vec<CosmosMsg<T>>,    
    /// The attributes that will be emitted as part of a "wasm" event
    pub attributes: Vec<Attribute>,
    pub data: Option<Binary>,
}

with

SubMsg<T = Empty> {
      id: String,
      msg: CosmosMsg<T>,
      gas_limit: Option<u64>,
}

Also for efficiency, I suggest using id: u64 instead of id: String, which is cheaper to ser/de and avoids heap allocations.

I am highly concerned about adding knowledge of gas values into the contract. This requires an immutable gas cost system. Changes in gas cost have broken existing contracts in the past (on Ethereum). Maybe we avoid the gas_limit for now, assuming the contract only emits messages with reasonable gas consumtion.

@ethanfrey
Copy link
Member Author

ethanfrey commented Feb 8, 2021

Separating these out into to different fields makes sense. I assume a minor typo above, where it should be pub sub_messages: Vec<SubMsg<T>> (and we don't need the Box anymore in SubMsg due to no circular dependencies). Other than that I agree with your suggestions, including u64 id.

@ethanfrey
Copy link
Member Author

ethanfrey commented Feb 8, 2021

Why gas_limit? This can be useful in quite a number of cases and should be provided by the end-user (not the app). We can define it as 'Cosmos SDK Gas' (much used in storage access not Wasm CPU). The reason this is important is that I may want to prevent such sub calls from triggering "out of gas".

Use case 1: "Ethereum Alarm Clock"

This was a project that was recently requested on discord. Basically, I submit a task and some token reward to an "Alarm Clock" contract, along with an expected time (range) to run it. If someone calls the Alarm Clock with sufficient gas during the time, it will (1) run your requested task and (2) send the pre-paid fees to the person calling the contract (to cover gas costs and more).

Now, we can do this right now, but if your requested task returns an error or uses some huge gas amount, it can return an error and abort the payment. If we use a sub_message to isolate it, a failed task can be marked as invalid and still provide a payout. But we have no gas limit we need to pay. If we use a sub_message and the original user provided a gas limit, we can call it with that. If we paid too little gas, it may still OutOfGas and revert the whole transaction. If we paid enough gas to covered the requested gas to the call, even if it needed more gas, we can get out payment (the user requested 200k gas for a call that needed 500k).

Use case 2: "Cron job"

Imagine a contract that is called every begin block and automatically performs a service like what we describe for the alarm clock (without off-chain caller). It may have some configured gas limit per block, and again, needs to allow the executed tasks to fail (and record that) rather than revert the transaction. It also wants some control over the gas usage of the sub_messages so they don't burn all the available gas of the cron job (which may wish to execute 10 messages). This is similar to above, but since the caller is on chain, it is even more important to provide these limits.

We can definitely add a note, that those gas_limits should not be hard-coded inside contracts, but provided from external users. If you really need to dispatch messages with a set value, it should be set in storage on InitMsg, and modifiable by some admin account - never hardcoded in the wasm binary.

@webmaster128
Copy link
Member

We can definitely add a note, that those gas_limits should not be hard-coded inside contracts, but provided from external users. If you really need to dispatch messages with a set value, it should be set in storage on InitMsg, and modifiable by some admin account - never hardcoded in the wasm binary.

This sounds like a good strategy. It also fits in the vision of multi-chain contracts where one blob runs on different chains, but must expect different prices for things.

@webmaster128
Copy link
Member

What's your current thinking regarding recursion and order of execution, @ethanfrey? I mean a sub message can emit sub messages itself.

@ethanfrey
Copy link
Member Author

ethanfrey commented Feb 8, 2021

What's your current thinking regarding recursion and order of execution, @ethanfrey? I mean a sub message can emit sub messages itself.

Good question. Currently, we do a depth-first strategy with messages. We call the first one, then call all sub-messages and so on. When finished, we go back up to the second message and recurse. I would like to keep this.

The question is do we execute the returned sub-messages or returned messages first. Executing the sub-message depth-first includes executing the callback to this contract. Assume contract C returns submessage S and message M, and they don't dispatch others, we have two choices:

  • C, S, C(S), M
  • C, M, S, C(S)

I am not certain which is better, but I am tending towards the first - sub-messages before messages. This means, we get and process the results of these points where we want feedback before dispatching the generic "fire and forget" type messges. I am open to any reasons why one ordering is better than the other. (This is the only downside of the new design, before the ordering was explicit by the contract dev)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants