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

One or multiple msg_response in Reply #2009

Closed
2 tasks
webmaster128 opened this issue Jan 30, 2024 · 5 comments · Fixed by #2032
Closed
2 tasks

One or multiple msg_response in Reply #2009

webmaster128 opened this issue Jan 30, 2024 · 5 comments · Fixed by #2032
Milestone

Comments

@webmaster128
Copy link
Member

This is how the data in the reply is currently handled in wasmd: https://github.com/CosmWasm/wasmd/blob/v0.50.0/x/wasm/keeper/msg_dispatcher.go#L103-L150. It has been implemented like that in the original implementation: https://github.com/CosmWasm/wasmd/pull/441/files

The list of datas is coming from here: https://github.com/CosmWasm/wasmd/blob/v0.50.0/x/wasm/keeper/handler_plugin.go#L69-L89. There the function

	// Encode converts wasmvm message to n cosmos message types
	Encode(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) ([]sdk.Msg, error)

turns a single msg wasmvmtypes.CosmosMsg into multiple []sdk.Msg. This was needed at some point in the past where we had a claim rewards message in cosmwasm that translated to a set withdrawl address + withdraw message. I wonder if this is still needed.

Then I wonder why each result again has a list of responses. As far as I can tell, the idea of message services is that each message leads to one response (e.g. MsgExecuteContract/MsgExecuteContractResponse, MsgSend/MsgSendResponse, ...)
Bildschirmfoto 2024-01-30 um 09 47 14

So open questions for me are

  • Do we still need to encode 1 wasmvmtypes.CosmosMsg to multiple sdk.Msg or is one sufficient?
  • Can the message handler type *sdk.Result in handleSdkMessage really contain multiple responses? Or is it just a list because it is also used for transactions that can contain multiple messages?
@webmaster128 webmaster128 added this to the 2.0.0 milestone Jan 30, 2024
@chipshort
Copy link
Collaborator

  1. It looks to me like all of our DefaultEncoders only return one message. What I don't know is if any chain provides their own message encoders / handler and returns multiple results.
    Surely they at the very least provide custom message support, but no idea if they ever have to execute multiple messages for that.
  2. I found this comment in the Cosmos SDK regarding that. And it seems to be correct, as the message service router only ever returns one response.

@webmaster128
Copy link
Member Author

webmaster128 commented Jan 30, 2024

Good stuff.

I was thinking about some more cases for 1.: here is a case where we created two SDK messages from one CosmWasm message. It was semantically buggy to do that but we might have such cases again in the future if we add IBC fees as fields to IbcMsg::SendPacket and IbcMsg::Transfer. Fees then would translate to a separate message emitted before the actual action as described in ICS-29.

Okay, good to assume 1 element, but no type safety.

So we end up with two nested loops, each is expected to only have one element. However, we never really know if it remains like that. So what I propose is keep the vector and flatten the results of the loops. Then the majority of users find their response in .msg_results[0] but we are safe for cases where we either emit more SDK messages or have more response elements per message.

@webmaster128
Copy link
Member Author

So what I propose is keep the vector and flatten the results of the loops.

See CosmWasm/wasmd#1800 for how this can look like in wasmd.

@chipshort
Copy link
Collaborator

The approach in CosmWasm/wasmd#1800 looks good to me. Once the wasmvm prerelease is done, we can finish that off.

@webmaster128
Copy link
Member Author

@chipshort could you close this ticket with a doc comment in cosmwasm-std explaining how the value is filled by wasmd such that contract developers know what to expect?

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