Wasmd does not sort coins when converting from CosmWasm Coins to SDK Coins

[apollo-sturdy]

Cosmos SDK expects its Coins object to be sorted alphabetically by denom, or else it will throw an invalid coins error.
In function ConvertWasmCoinsToSdkCoins when converting from CosmWasm coins to SDK coins, they are not sorted, which means that they are also not sorted when encoding CosmosMsgs that come from Responses returned from contract executions. This means that all contracts must ensure to sort coins before issuing responses with CosmosMsgs that contain native coins. This is obviously not ideal and leads to bugs when contract authors forget to sort the coins.

My proposed solution is to simply sort the coins when converting them into SDK coins. I have implemented a fix here: #1117

[webmaster128]

Looking at the code, wasmd does not do any normalization of the coins. That means they need to pass the Cosmos SDK Coins bank MsgSend validation directly. This comes with the following requirements:

1. All denoms are valid
2. Each amount is > 0
3. No denom is duplicated
4. Denoms are sorted

From those, 1. cannot be auto-fixed but 2.-4. can. Now if we start auto-fixing coins in wasmd we should auto-fix all of them.

However, this creates an inconsistency between the message types that have an explicit wrapper and stargate messages which we do not operate on. So you can also say that the contract should take care of creating the correct coins list. This will become super easy with the coins struct.

[apollo-sturdy]

The coins struct looks great! Will info.funds be updated to use this type? And CosmosMsg variants use it?

I agree that it makes sense here to fix everything that can be fixed. Would you want me to update the PR to fix requirement 2 and 3?

As for stargate messages, my proposed fix only applies validation when converting CosmWasm coins to SDK Coins, and a Stargate message wouldn't contain any CosmWasm Coins, so is there really an inconsistency here?

Another reason I think it makes sense to implement a fix for the conversion of CosmWasm Coins to SDK coins is that as a CosmWasm developer who does not know the inner workings of the Cosmos SDK, when you get the error message invalid coins you immediately think that you have used an incorrect denom, after all why would anything else give an error? Isn't part of the idea of using CosmWasm that you shouldn't need to know about the inner workings of the SDK?

[webmaster128]

I'd like to hear what @alpe and @ethanfrey say before investing too much in the implementation. wasmd is not really my home codebase.

Will info.funds be updated to use this type? And CosmosMsg variants use it?

I don't think this will happen any time soon since Coins is a map, not a list. So it would be heavily API breaking change. It is currently envisioned as an in-memory instance for Coin handling that can be exported as a correctly formatted Coin list. So not going to be a full solution.

As for stargate messages, my proposed fix only applies validation when converting CosmWasm coins to SDK Coins, and a Stargate message wouldn't contain any CosmWasm Coins, so is there really an inconsistency here?

So far I did not consider CosmWasm Coins a different thing than Cosmos SDK Coins, but I can absolutely relate to the argument.

a CosmWasm developer who does not know the inner workings of the Cosmos SDK, when you get the error message invalid coins you immediately think that you have used an incorrect denom, after all why would anything else give an error? Isn't part of the idea of using CosmWasm that you shouldn't need to know about the inner workings of the SDK?

This sounds compelling :)

[alpe]

This was fixed in #1117 💯

[webmaster128]

It's not. None of this was addressed in the PR there and it would be good to talk about those things as they affect API definition of cosmwasm.

[apollo-sturdy]

Happy to create another PR to resolve issues 2 and 3 outlined above if that's what we want! Let me know.

[webmaster128]

Yeah I think it would be cool to ship the whole auto-fixing story (2.-4.) for the user in the next wasmd release. The change in #1117 is consensus breaking already (turns errors into non-errors) so we can make it worth it.

I think it's important to add tests to ConvertWasmCoinsToSdkCoins that show the behaviour we want here. The tests from https://github.com/cosmos/cosmos-sdk/blob/9bf027460bae724bbc1fddd3541248b7fe18ab2c/types/coin_test.go#L642-L845 might be a good inspiration for a start. But we'd need always CosmWasm Coin list inputs and expected Cosmos SDK Coins as output.

[webmaster128]

@apollo-sturdy do you think you can work on a PR implementing the other two auto-fixers and tests? Not requeired this year but it would be great to get the whole feature as a bundle for the next wasmd 0.31.0 release.