ibc: TimeoutOnClose support #7066
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7066 +/- ##
==========================================
- Coverage 55.60% 54.60% -1.00%
==========================================
Files 457 547 +90
Lines 27440 37130 +9690
==========================================
+ Hits 15257 20275 +5018
- Misses 11083 15196 +4113
- Partials 1100 1659 +559 |
|
I can take over this pr, I think it makes sense to not add an additional callback and use cc/ @cwgoes thoughts? Also, this was not specified in the ICS spec so I think it should be updated there |
…6993-timeoutonclose
…6993-timeoutonclose
| @@ -266,28 +266,6 @@ func (k Keeper) TimeoutOnClose( | |||
| return err | |||
| } | |||
|
|
|||
| k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | |||
There was a problem hiding this comment.
I think this was an unreachable bug that we didn't close ordered channels upon timeout
|
|
||
| // Type implements sdk.Msg | ||
| func (msg MsgTimeoutOnClose) Type() string { | ||
| return "timeout_on_close_packet" |
There was a problem hiding this comment.
it is a little strange to use timeout_on_close_packet for MsgTimeoutOnClose ?
colin-axner
requested review from
aaronc,
alessio,
alexanderbez,
clevinson and
jgimeno
as code owners
| }, true}, | ||
| {"success: UNORDERED timeout out of order packet", func() { | ||
| // setup uses an UNORDERED channel | ||
| clientA, clientB, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) |
There was a problem hiding this comment.
nit, without the comment, it is not intuitive that the channels are unordered. Maybe consider renaming it or adding a boolean arg?
There was a problem hiding this comment.
I agree, I'm thinking we will likely have more channel types in the future, so a bool or different function names wouldn't make sense. Instead I could add a third arg that would be like channeltypes.ORDERED. This is a fairly large line diff change and I think would be easier to review in a followup, I can add it to the wishlist and address it when I begin to untangle ibc-transfer from testing.
|
ACK. @colin-axner can you approve and merge the PR? |
| } | ||
|
|
||
| // Perform application logic callback | ||
| // NOTE: MsgTimeout and MsgTimeoutOnClose use the same "OnTimeoutPacket" |
There was a problem hiding this comment.
For now, I think using the same callback is fine
Description
closes: #6993
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes