Add StdAck

[webmaster128]

Closes #1512

After a bit of back and forth, this is now very minimal and non-opinionated. It ensures compatibility with the standard ack types from ICS-20 and ICS-27 amonst others.

In noislabs/nois-contracts#279 you see how this can be used by projects to avoid the need to have their own ack type. There you can also see that the struct -> binary conversion (JSON serialitzation) is done outside of the ack. I.e. this type does not know or care what kind of bytes you store in the success case.

[0xekez]

i'm not convinced the ICS-20 format should be standardized. why encoded to base64 if you can just send JSON in the result, for example: https://git.sr.ht/~ekez/ica-secret/tree/main/item/packages/polytone/src/ack.rs#L5

[0xekez]

it honestly drives me a little insane that the SDK forces you to use the ICS-20 format for error variants by wrapping it up like this: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/acknowledgement.go#L35

[webmaster128]

I don't have too much insight here. I copied this from @ethanfrey's code and looked up references for the decisions being made by others in the past.

In general I think it's good to have a generic success/error wrapper that does not bother about the application specific sucess data and just uses binary. This allows us to have one layer that works with both ICS-20 and CosmWasm contract acks.

[ethanfrey]

I understand it is one level of nesting, but we do that often with things like cw20::Send or dao dao ExecuteMsg::Propose or WasmMsg::Execute. Using Binary for "arbitrary json message to a contract we don't interpret at this level.

Making a StdAck format is saying "Success if protocol-dependent, a higher level can parse to detect success/error without any protocol knowledge". I agree something like Go's json.RawMessage would be great, but we don't really have a good alternative in our Rust libraries. (There is one in serde-json under a feature flag but it brings in floats).

When I see:

#[cw_serde]
pub enum Callback {
    /// Data returned from the host chain. Index n corresponds to the
    /// result of executing the nth message/query.
    Success(Vec<Option<Binary>>),
    /// The first error that occured while executing the requested
    /// messages/queries.
    Error(String),
}

I see a very specific app-result, which is great, but not the standard.

Note, that the Error variant is equivalent to the StdAck one in JSON encoding... so if you just rely on x/wasm handling to auto-encode your errors, then it will be compatible (as x/wasm doesn't auto-encode successes and requires you to properly encode).

[0xekez]

@ethanfrey, thanks for the thoughts. I think nesting base64 is great when the type isn't known at compile time (needs to be an interface => "generic at runtime" => base64). In this case, I think the advantage is that it might normalize the ACK data and make it more compact.

The ACK format is known at compile time. For example, you might write:

#[cw_serde]
pub enum Ack<T> {
    Result(T),
    Error(String),
}

pub fn ack_success<T: Serialize>(c: T) -> Binary {
    let res = Ack::Result(c);
    to_binary(&res).unwrap()
}

// etc..

This is a bit of a bikeshed though, and thinking about it more I don't think there is anything wrong with the base64 approach so fine by me either way.

[webmaster128]

The ACK format is known at compile time. For example, you might write:

Isn't that the same as Binary::from(StdAck::success_json(c)) from this PR?

---

One thing I was thinking about all the time is how to better separate the JSON serde from the base Ack to get great JSON support but also respect that not all protocols use JSON in the ack success. And looking of the discussion here we can do that plus get better type-safety by using a generic type which can still be StdAck compatible.

// A newtype with an extra PhantomData
#[repr(transparent)]
#[serde(transparent)]
pub struct JsonAck<T> {
    // private members
    ack: StdAck,
    marker: PhantomData<*const T>,
}

impl<T: Serialize + DeserializeOwned> JsonAck {
    // construct from T
    // unwrap into T
    // map to Result<T, String>
}

[webmaster128]

Okay this struct JsonAck<T> idea is not ideal because you can't match it.

So we better just have StdAck and Ack<T> separately, both having the compatible base64 encoding in the success case.

[webmaster128]

it honestly drives me a little insane that the SDK forces you to use the ICS-20 format for error variants by wrapping it up like this: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/acknowledgement.go#L35

This is just an constructor for the Acknowledgement type of ibc-go (which serializes to JSON). This constructor may or may not be used. What do you mean by "SDK" in this case? wasmd?

In c588c18 you see how you can prevent the creation of such error acks. This should allow you to use whichever ack type you need. See the rest of that PR for more context why we are working on this.

[0xekez]

This should allow you to use whichever ack type you need.

If I return Never as my error variant, but my contract panics on ibc_packet_receive will wasmd return {"error": "codespace wasm: 5"} to the packet sender? What if, for example, I make a protocol that has an error variant like Vec<String> (for example, if in Polytone I didn't want to bail out on the first message execution failure, and instead wanted to return a list of all errors that occured when executing Vec<CosmosMsg>)? It feels to me that the underlying system should never make assumptions about the shape of ACKs (as those are opaque in the protocol).

[webmaster128]

If I return Never as my error variant, but my contract panics on ibc_packet_receive will wasmd return {"error": "codespace wasm: 5"} to the packet sender?

Until wasmd 0.31, yes. From 0.32 onwards, panics abort the transaction and no ack is written. See more on that in #1646.

It feels to me that the underlying system should never make assumptions about the shape of ACKs (as those are opaque in the protocol)

Since in some cases wasmd creates error acks, it has to make those assumptions. #1646 is your friend. Happy to answer questions over there.

[webmaster128]

why encoded to base64 if you can just send JSON in the result

I was thinking about this a bit and came up with JsonAck, implementing whis approach. It's nicer than I originally thought, especially since JsonAck is compatible with the ICS-20 acknowledgement type. See #1657.

[webmaster128]

Updated the main description and consider this ready now.

[chipshort]

I don't fully understand the whole discussion, but the code looks good to me.

[chipshort]

Not a fan of assert!(matches!(...))?

[webmaster128]

Does this allow me to compare the data value too?

[chipshort]

Yes, like this: assert!(matches!(success, StdAck::Success(data) if data == b"foo"))

[webmaster128]

🧙‍♂️