Merge pull request #885 from CosmWasm/on-packet-recv-error-handling

Add a section on encoding IBCReceive errors

Author: webmaster128

IBC.md

@@ -254,7 +254,7 @@ Protobuf encoders for it as the protocol requires.
 
 After a contract on chain A sends a packet, it is generally processed by the
 contract on chain B on the other side of the channel. This is done by executing
-the following callback on chain B:
+the following entry point on chain B:
 
 ```rust
 #[entry_point]
@@ -265,24 +265,35 @@ pub fn ibc_packet_receive(
 ) -> StdResult<IbcReceiveResponse> { }
 ```
 
-Note the different return response here (`IbcReceiveResponse` rather than
-`IbcBasicResponse`)? This is because it has an extra field
-`acknowledgement: Binary`, which must be filled out. That is the response bytes
-that will be returned to the original contract, informing it of failure or
-success. (Note: this is vague as it will be refined in the next PR)
+This is a very special entry point as it has a unique workflow. (Please see the
+[Acknowledging Errors section](#Acknowledging-Errors) below to understand it
+fully).
 
-Here is the
-[`IbcPacket` structure](https://github.com/CosmWasm/cosmwasm/blob/v0.14.0-beta4/packages/std/src/ibc.rs#L129-L146)
-that contains all information needed to process the receipt. You can generally
-ignore timeout (this is only called if it hasn't yet timed out) and sequence
-(which is used by the IBC framework to avoid duplicates). I generally use
-`dest.channel_id` like `info.sender` to authenticate the packet, and parse
-`data` into a `PacketMsg` structure, using the same encoding rules as we
-discussed in the last section.
+Also note the different return response here (`IbcReceiveResponse` rather than
+`IbcBasicResponse`). This is because it has an extra field
+`acknowledgement: Binary`, which must be filled out. All successful message must
+return an encoded `Acknowledgement` response in this field, that can be parsed
+by the sending chain.
 
-After that you can process `PacketMsg` more or less like an `ExecuteMsg`,
-including calling into other contracts. The only major difference is that you
-must return Acknowledgement bytes in the protocol-specified format.
+The
+[`IbcPacket` structure](https://github.com/CosmWasm/cosmwasm/blob/v0.14.0-beta4/packages/std/src/ibc.rs#L129-L146)
+contains all information needed to process the receipt. This info has already
+been verified by the core IBC modules via light client and merkle proofs. It
+guarantees all metadata in the `IbcPacket` structure is valid, and the `data`
+field was written on the remote chain. Furthermore, it guarantees that the
+packet is processed at most once (zero times if it times out). Fields like
+`dest.channel_id` and `sequence` have a similar trust level to `MessageInfo`,
+which we use to authorize normal transactions. The `data` field should be
+treated like the `ExecuteMsg` data, which is only as valid as the entity that
+signed it.
+
+You can generally ignore `timeout_*` (this entry point is only called if it
+hasn't yet timed out) and `sequence` (which is used by the IBC framework to
+avoid duplicates). I generally use `dest.channel_id` like `info.sender` to
+authenticate the packet, and parse `data` into a `PacketMsg` structure, using
+the same encoding rules as we discussed in the last section. After that you can
+process `PacketMsg` more or less like an `ExecuteMsg`, including calling into
+other contracts.
 
 ```rust
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
@@ -305,10 +316,157 @@ pub struct IbcPacket {
 }
 ```
 
-TODO: explain how to handle/parse errors (As part of
-https://github.com/CosmWasm/cosmwasm/issues/762)
-
-##### Standard Acknowledgement Format
+##### Acknowledging Errors
+
+A major issue that is unique to `ibc_packet_receive` is that it is expected to
+often reject an incoming packet, yet it cannot abort the transaction. We
+actually expect all state changes from the contract (as well as dispatched
+messages) to be reverted when the packet is rejected, but the transaction to
+properly commit an acknowledgement with encoded error. In other words, this "IBC
+Handler" will error and revert, but the "IBC Router" must succeed and commit an
+acknowledgement message (that can be parsed by the sending chain as an error).
+
+The atomicity issue was first
+[analyzed in the Cosmos SDK implementation](https://github.com/cosmos/ibc-go/issues/68)
+and refined into
+[changing semantics of the OnRecvPacket SDK method](https://github.com/cosmos/ibc-go/issues/91),
+which was
+[implemented in April 2021](https://github.com/cosmos/ibc-go/pull/107), likely
+to be released with Cosmos SDK 0.43 or 0.44. Since we want the best,
+future-proof interface for contracts, we will use an approach inspired by that
+work, and add an adapter in `wasmd` until we can upgrade to a Cosmos SDK version
+that implements this.
+
+After quite some
+[discussion on how to encode the errors](https://github.com/CosmWasm/cosmwasm/issues/762),
+we struggled to map this idea to the CosmWasm model. However, we also discovered
+a deep similarity between these requirements and the
+[submessage semantics](./SEMANTICS.md#submessages). It just requires some
+careful coding on the contract developer's side to not throw errors. This
+produced 3 suggestions on how to handle errors and rollbacks _inside
+`ibc_packet_receive`_
+
+1. If the message doesn't modify any state directly, you can simply put the
+   logic in a closure, and capture errors, converting them into error
+   acknowledgements. This would look something like the
+   [main dispatch loop in `ibc-reflect`](https://github.com/CosmWasm/cosmwasm/blob/cd784cd1148ee395574f3e564f102d0d7b5adcc3/contracts/ibc-reflect/src/contract.rs#L217-L248):
+
+   ```rust
+    (|| {
+        // which local channel did this packet come on
+        let caller = packet.dest.channel_id;
+        let msg: PacketMsg = from_slice(&packet.data)?;
+        match msg {
+            PacketMsg::Dispatch { msgs } => receive_dispatch(deps, caller, msgs),
+            PacketMsg::WhoAmI {} => receive_who_am_i(deps, caller),
+            PacketMsg::Balances {} => receive_balances(deps, caller),
+        }
+    })()
+    .or_else(|e| {
+        // we try to capture all app-level errors and convert them into
+        // acknowledgement packets that contain an error code.
+        let acknowledgement = encode_ibc_error(format!("invalid packet: {}", e));
+        Ok(IbcReceiveResponse {
+            acknowledgement,
+            submessages: vec![],
+            messages: vec![],
+            attributes: vec![],
+        })
+    })
+   ```
+
+2. If we modify state with an external call, we need to wrap it in a
+   `submessage` and capture the error. This approach requires we use _exactly
+   one_ submessage. If we have multiple, we may commit #1 and rollback #2 (see
+   example 3 for that case). The main point is moving `messages` to
+   `submessages` and reformating the error in `reply`. Note that if you set the
+   `Response.data` field in `reply` it will override the acknowledgement
+   returned from the parent call. (See
+   [bottom of reply section](./SEMANTICS.md#handling-the-reply)). You can see a
+   similar example in how
+   [`ibc-reflect` handles `receive_dispatch`](https://github.com/CosmWasm/cosmwasm/blob/eebb9395ccf315320e3f2fcc526ee76788f89174/contracts/ibc-reflect/src/contract.rs#L307-L336).
+   Note how we use a unique reply ID for this and use that to catch any
+   execution failure and return an error acknowledgement instead:
+
+   ```rust
+   fn receive_dispatch(
+       deps: DepsMut,
+       caller: String,
+       msgs: Vec<CosmosMsg>,
+   ) -> StdResult<IbcReceiveResponse> {
+       // what is the reflect contract here
+       let reflect_addr = accounts(deps.storage).load(caller.as_bytes())?;
+
+       // let them know we're fine
+       let acknowledgement = to_binary(&AcknowledgementMsg::<DispatchResponse>::Ok(()))?;
+       // create the message to re-dispatch to the reflect contract
+       let reflect_msg = ReflectExecuteMsg::ReflectMsg { msgs };
+       let wasm_msg = wasm_execute(reflect_addr, &reflect_msg, vec![])?;
+
+       // we wrap it in a submessage to properly report errors
+       let sub_msg = SubMsg {
+           id: RECEIVE_DISPATCH_ID,
+           msg: wasm_msg.into(),
+           gas_limit: None,
+           reply_on: ReplyOn::Error,
+       };
+
+       Ok(IbcReceiveResponse {
+           acknowledgement,
+           submessages: vec![sub_msg],
+           messages: vec![],
+           attributes: vec![attr("action", "receive_dispatch")],
+       })
+   }
+
+   #[entry_point]
+   pub fn reply(deps: DepsMut, _env: Env, reply: Reply) -> StdResult<Response> {
+      match (reply.id, reply.result) {
+         (RECEIVE_DISPATCH_ID, ContractResult::Err(err)) => Ok(Response {
+            data: Some(encode_ibc_error(err)),
+            ..Response::default()
+         }),
+         (INIT_CALLBACK_ID, ContractResult::Ok(response)) => handle_init_callback(deps, response),
+         _ => Err(StdError::generic_err("invalid reply id or result")),
+      }
+   }
+   ```
+
+3. For a more complex case, where we are modifying local state and possibly
+   sending multiple messages, we need to do a self-call via submessages. What I
+   mean is that we create a new `ExecuteMsg` variant, which returns an error if
+   called by anyone but the contract itself
+   (`if info.sender != env.contract.address { return Err() }`). When receiving
+   the IBC packet, we can create a submessage with `ExecuteMsg::DoReceivePacket`
+   and any args we need to pass down.
+
+   `DoReceivePacket` should return a proper acknowledgement payload on success.
+   And return an error on failure, just like a normal `execute` call. However,
+   here we capture both success and error cases in the `reply` handler (use
+   `ReplyOn::Always`). For success, we return this data verbatim to be set as
+   the packet acknowledgement, and for errors, we encode them as we did above.
+   There is not any example code using this (yet), but it is just recombining
+   pieces we already have. For clarity, the `reply` statement should look
+   something like:
+
+   ```rust
+   #[entry_point]
+   pub fn reply(_deps: DepsMut, _env: Env, reply: Reply) -> StdResult<Response> {
+       if reply.id != DO_IBC_RECEIVE_ID {
+           return Err(StdError::generic_err("invalid reply id"));
+       }
+       let data = match reply.result {
+           ContractResult::Ok(response) => response.data,
+           ContractResult::Err(err) => Some(encode_ibc_error(err)),
+       };
+       Ok(Response {
+           data,
+           ..Response::default()
+       })
+   }
+   ```
+
+##### Standard Acknowledgement Envelope
 
 Although the ICS spec leave the actual acknowledgement as opaque bytes, it does
 provide a recommendation for the format you can use, allowing contracts to
@@ -330,12 +488,18 @@ message Acknowledgement {
 
 Although it suggests this is a Protobuf object, the ICS spec doesn't define
 whether to encode it as JSON or Protobuf. In the ICS20 implementation, this is
-JSON encoded when returned from a contract. Given that, we will consider this
-structure, JSON-encoded, to be the "standard" acknowledgement format.
+JSON encoded when returned from a contract. In ICS27, the authors are discussing
+using a Protobuf-encoded form of this structure.
+
+Note that it leaves the actual success response as app-specific bytes where you
+can place anything, but does provide a standard way for an observer to check
+success-or-error. If you are designing a new protocol, I encourage you to use
+this struct in either of the encodings as the acknowledgement envelope.
 
 You can find a
 [CosmWasm-compatible definition of this format](https://github.com/CosmWasm/cosmwasm-plus/blob/v0.6.0-beta1/contracts/cw20-ics20/src/ibc.rs#L52-L72)
-as part of the `cw20-ics20` contract.
+as part of the `cw20-ics20` contract, along with JSON-encoding. Protobuf
+encoding version can be produced upon request.
 
 #### Receiving an Acknowledgement
 

contracts/ibc-reflect/examples/schema.rs

@@ -4,8 +4,8 @@ use std::fs::create_dir_all;
 use cosmwasm_schema::{export_schema, export_schema_with_title, remove_schemas, schema_for};
 
 use ibc_reflect::msg::{
-    AcknowledgementMsg, BalancesResponse, DispatchResponse, ExecuteMsg, InstantiateMsg, PacketMsg,
-    QueryMsg, WhoAmIResponse,
+    AcknowledgementMsg, BalancesResponse, DispatchResponse, InstantiateMsg, PacketMsg, QueryMsg,
+    WhoAmIResponse,
 };
 
 fn main() {
@@ -14,7 +14,6 @@ fn main() {
     create_dir_all(&out_dir).unwrap();
     remove_schemas(&out_dir).unwrap();
 
-    export_schema(&schema_for!(ExecuteMsg), &out_dir);
     export_schema(&schema_for!(InstantiateMsg), &out_dir);
     export_schema(&schema_for!(QueryMsg), &out_dir);
     export_schema(&schema_for!(PacketMsg), &out_dir);