add channel freeze #761
add channel freeze #761
Conversation
* make min spread to 100% to disable swap * move fork code to app BeginBlocker * add channel freeze (#761) * add channel freeze * save channels Co-authored-by: Sunny Aggarwal <[email protected]>
| if !found { | ||
| panic("channel-1 not found") | ||
| } | ||
| channel.State = ibcchanneltypes.CLOSED |
There was a problem hiding this comment.
This allows the channel to be closed on Osmosis as well (via ChanCloseConfirm). This is fine, but to recover it on Osmosis would require similar code to this. I think it is unlikely anyone performs ChanCloseConfirm (I am not sure relayers support it).
An ante handler could have been added instead. If the msg is a MsgTransfer and the source port/channel match the ones above, then return an error.
Osmosis could consider adding this ante handler at least on checktx for ChanCloseConfirm, but I think it is unlikely ChanCloseConfirm will be called. Probably easier just to manually reopen the channel
There was a problem hiding this comment.
An alternative solution would be to use a fork of ibc-go and add the check here. That way you don't adjust the channel state (and cause unwanted sideaffects), but still disable sends. I assume it is too late for this though
There was a problem hiding this comment.
I think it is fine that the channel gets closed on Osmosis as well, and it requires a manual intervention on both ends to reopen.
Also if you disable sends, you will disable sending for all channels which doesn't seem to be the intent of this PR which only closes the channels to DEX chains.
This will allow relayers to still process timeouts so pending sends can be refunded to original users. This may require relayers to support the TimeoutOnCloseMsg but it is not time sensitive per se.
Any cross chain funds (ie. Terra on Osmosis and Osmo on Terra) will be stuck on the remote chain until/unless the channel reopens.
There was a problem hiding this comment.
@colin-axner does no channel close on Osmosis mean UST can be withdrawn back to Terra?
There was a problem hiding this comment.
no, it will fail on RECV: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/keeper/packet.go#L157
Summary of changes
Report of required housekeeping
(FOR ADMIN) Before merging