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

Allow submessage callbacks only on error #854

Closed
ethanfrey opened this issue Mar 30, 2021 · 9 comments · Fixed by #857
Closed

Allow submessage callbacks only on error #854

ethanfrey opened this issue Mar 30, 2021 · 9 comments · Fixed by #857
Milestone

Comments

@ethanfrey
Copy link
Member

When we make a submessage, we can handle both success and errors from the released message. However, this comes at a cost of paying the instantiation gas cost of the original module for each submessage.

For some use cases, eg. mapping errors in #762, we only want to handle if an error arises, and don't care about the return values. Since we assume errors are not the normal case, we add a lot of gas overhead to all the callbacks we ignore. It would be more efficient to be able to filter these out in Go, and just get callbacks on error if that is what we want.

In particular, I want to extend SubMsg with one more field:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct SubMsg<T = Empty>
where
    T: Clone + fmt::Debug + PartialEq + JsonSchema,
{
    pub id: u64,
    pub msg: CosmosMsg<T>,
    pub gas_limit: Option<u64>,
    pub only_error: Option<bool>,
}

If only_error is set to true, then we only make the response callback if there was an error (incl running out of gas), otherwise, just handle it like a message.

I made only_error as an Option because I feel it is usually unset and we can make the normal JSON smaller (and use less gas) by simply omitting the field. Some(true) may be a bit wordy, but None and false are the same, which is the typical case. (I can change it to just bool if there is disagreement)

@ethanfrey ethanfrey added this to the 0.14.0 milestone Mar 30, 2021
@webmaster128
Copy link
Member

Sounds good. I would just make clear that this is about the reply, not about the SubMsg itself. What about reply_on_success?

I made only_error as an Option because I feel it is usually unset and we can make the normal JSON smaller (and use less gas) by simply omitting the field. Some(true) may be a bit wordy, but None and false are the same, which is the typical case. (I can change it to just bool if there is disagreement)

I would avoid representing a a two state flag in a 3 state type. Option's in Rust do not allow omitting them so they are not like field?: boolean in TypeScript. Also, only_error: None is serialized as "only_error":null, which saves a single byte compated to "only_error":false.

@ethanfrey
Copy link
Member Author

Option's in Rust do not allow omitting them so they are not like field?: boolean in TypeScript.

Ah, I did not know that, good point.

@webmaster128
Copy link
Member

no_message_is_good_message: bool

@ethanfrey
Copy link
Member Author

ethanfrey commented Mar 30, 2021

Sounds good. I would just make clear that this is about the reply, not about the SubMsg itself. What about reply_on_success?

Hmm... I don't see a use for that one (basically no need to optimize gas for unhandled errors). But maybe you could convince me otherwise. Oh, wait you mean for a name for this field?

I like no_message_is_good_message: bool (default false) but it does seem long

@webmaster128
Copy link
Member

webmaster128 commented Mar 30, 2021

Oh, wait you mean for a name for this field?

Yes. I think it better describes what you build. You want to toggle the reply (i.e. execution of the reply entry point) based on success/error, not the sub message execution.

@ethanfrey
Copy link
Member Author

I will think more on naming, but it seems that we are in agreement on everything but a good name

@webmaster128
Copy link
Member

reply_on_success negates your flag. It would be set to true in most cases. I think it is more direct than only_error = ignore_reply_on_error.

@ethanfrey
Copy link
Member Author

If we are going for complete, let's make a 3 way flag: reply_on: "all" | "success" | "error" in ts syntax. (I can make a more rusty version if you like the idea)

@webmaster128
Copy link
Member

Yeah, nice.

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