Shielded refund design

[sug0]

Currently, Namada does not support the shielded refunding of some tx. However, it may be possible to do so, with the following:

1. The user optionally specifies a shielded refund MASP transaction option via the CLI/SDK.
2. The CLI/SDK generates a shielding transaction associated with the funds unshielded via IBC.
3. The protocol intercepts the ICS-20 send_transfer_execute handler from ibc-rs, and stores the generated shielding transaction associated with this port, channel and sequence-number triplet.
4. Upon receiving an ICS-20 acknowledgement, the protocol can intercept the on_acknowledgement_packet_execute handler from ibc-rs, and retrieve the shielding tx associated with the port, channel and sequence-number triplet retrieved from the packet.
5. In case the ack is a success, the shielding transfer is deleted from storage with no further action.
6. Otherwise, the shielding tx's state changes are applied, to reshield the transferred funds.

[sug0]

@yito88 @grarco

[grarco]

I don't think the reason we don't have shielded refunds is a matter of how we handle the tx from the ibc perspective. If I remember right we tried two ways:

1. Letting the relayer construct the shielding transaction for the user (this was actually the logic applied to any shielding operation coming from an external chain, being it a refund or not)
2. Attaching the refunding tx to the memo (which is a slight variation of what you are suggesting)

The problem with solution 1 is that we can verify the amount and the token but we cannot verify the actual recipient, so the relayer could misbehave and target a different address.
The problem with solution 2, which I believe also applies to the solution you are suggesting, is that there's a chance we hit a masp epoch change in between the construction of the shielding tx and its execution: if the token involved in the transaction is incentivized in the pool, its epoch won't match the current one and the transaction will be rejected by the MASP vp.

@yito88, @murisi please correct me if I've said something wrong

[sug0]

Ah I see, you are correct about point 2. But we could still fallback to a transparent refund, in case the masp epoch had changed. This would improve the UX a bit. Anyway, including the refund shielding tx as the memo does sound more simple.

Something like:

{
  "namada": {
    "ibc_shielding_refund": {
      "shielding_data": "<... shielding tx data>",
      "shielded_at_masp_epoch": 12,
      "fallback_refund_taddr": "tnam1..."
    }
  }
}

This can be implemented in a fairly straightforward way over on_acknowledgement_packet_execute, like we have for some other IBC middlewares (see: https://github.com/heliaxdev/ibc-middleware).

If we could map packet sequence numbers to masp epochs, then we could even remove shielded_at_masp_epoch from the memo, to save a bit of gas.

[yito88]

Sounds good! We could do that without the memo if the shielding transaction is stored in the storage associated with port, channel, sequence, and masp_epoch. The fallback (transparent) refund target is already set to the sender.

[sug0]

just another note: we also need to handle timeouts. the refund logic under timeouts is the same as when we get an ack error

[sug0]

@yito88 if you take this on, could you please use #4134 as a base?

[grarco]

Ah I see, you are correct about point 2. But we could still fallback to a transparent refund, in case the masp epoch had changed. This would improve the UX a bit. Anyway, including the refund shielding tx as the memo does sound more simple.

Something like:

{
  "namada": {
    "ibc_shielding_refund": {
      "shielding_data": "<... shielding tx data>",
      "shielded_at_masp_epoch": 12,
      "fallback_refund_taddr": "tnam1..."
    }
  }
}

This can be implemented in a fairly straightforward way over on_acknowledgement_packet_execute, like we have for some other IBC middlewares (see: https://github.com/heliaxdev/ibc-middleware).

If we could map packet sequence numbers to masp epochs, then we could even remove shielded_at_masp_epoch from the memo, to save a bit of gas.

I think you could also avoid adding the shielding epoch, you can just try to execute the refund shielding transaction and fallback on the disposable address on failure (even though we's need to be careful with the WriteLog state in this case). We might also want to have a look at #4095 to improve on the chances of hitting a masp epoch change (even though in the context of an IBC tx this could be harder to achieve in terms of timing).

Also @sug0, why is this breaking:tx?

Some other thoughts on this since it seems like the main goal of this is to improve on UX:

1. The client will be a bit slower at producing the ibc transaction because of the need to construct this refund shielding section
2. The size of the transaction will be larger with an increase in the gas cost for the user and in storage consumption for the nodes
3. There might be the need to update the masp indexer and the shielded-sync logic

In case of a failing IBC tx and assuming no change in the masp epoch this solution is indeed an improvement, but it looks to me like we might be adding some cost to the happy path trying to improve on the unhappy one (essentially producing and storing a shielding transaction that might not be needed). I guess this could depend on the ratio of failing shielded actions which I'm not sure we can estimate right now