Ics20 same ack handling as ibctransfer #653
Conversation
contracts/cw20-ics20/src/ibc.rs
Outdated
| @@ -338,6 +336,9 @@ fn on_packet_failure( | |||
| ) -> Result<IbcBasicResponse, ContractError> { | |||
| let msg: Ics20Packet = from_binary(&packet.data)?; | |||
|
|
|||
| // undo the balance update | |||
| reduce_channel_balance(deps.storage, &packet.src.channel_id, &msg.denom, msg.amount)?; | |||
There was a problem hiding this comment.
My concern would be: What if, for whatever reason, the packet failure message never arrives?
Not sure if this can be abused, as the successful deposit on the other side is the one crediting the funds. But still, if the packet failure never arrives, the balance would be left in an inconsistent state, resulting in lost funds.
There was a problem hiding this comment.
It should eventually arrive. If nothing out via "timeout".
If this never gets relayed, the senders tokens remained locked in the contract. Once the timeout arrives, they are paid back.
ethanfrey
force-pushed
the
ics20-same-ack-handling-as-ibctransfer
branch
from
74ee47c to
2ea606b
Compare
| @@ -32,15 +32,20 @@ pub struct Ics20Packet { | |||
| pub receiver: String, | |||
| /// the sender address | |||
| pub sender: String, | |||
| /// used only by us to control ack handling | |||
| pub v: Option<u32>, | |||
There was a problem hiding this comment.
I created an issue about this #662
There was a problem hiding this comment.
Yeah, I found this and had to revert it.
Turns out ibctransfer packet uses a super-strict JSON parser that fails on any unknown fields.
It seems to me that this is undesired behavior, but that is another discussion.
I need a different format and migration step to do a new release of this, very correct. I was out for a while.
Use the same handling as ibctransfer, where we update local balancer counter when sending, rather than wait for a success ack.
This works for the cases where relayer are slow to relay success acks (or never do it). It also allows a much more reliable and quicker round trip of send, return without waiting for the acks to be relayed even in the normal case.
Same behavior as ibctransfer, I don't need to be more conservative than that.