(test) Add tests for callback execution when forwarding a packet #6805
Conversation
| @@ -0,0 +1,182 @@ | |||
| package ibccallbacks_test | |||
There was a problem hiding this comment.
I had to add a new file here in the callbacks module because it's the only module in which we can check the callbacks counter. This required setting up a new, small test suite that creates 3 chains.
| testCases := []struct { | ||
| name string | ||
| testMemo string | ||
| expCallbackMapOnChainB map[callbacktypes.CallbackType]int |
There was a problem hiding this comment.
We discussed this with @chatton and our understanding is that when the memo specifies src_callback we expect the callback to be executed on B and not on C. basically when forwarding we ensure that all callbacks are executed on either the last hop (C) or the one immediately before that (B).
If that's not the case, please let me know! I was surprised to see some callbacks executed on B but after some reasoning it started to make sense.
There was a problem hiding this comment.
With the current implementation it makes sense that this would be the case.. but it is a little weird imo.
Wonder if we should have a testcase for ensuring that the send_packet callback is never invoked on chainA
There was a problem hiding this comment.
yea very strange, but I guess not much we can do. Would be great to add into the docs somewhere? With multi packetdata, it should become more explicit when and where callbacks are executed (hopefully 😅)
There was a problem hiding this comment.
I opened #6883 to ensure we document this properly
| expCallbackMapOnChainC: map[callbacktypes.CallbackType]int{}, | ||
| }, | ||
| { | ||
| name: "recv callback", |
There was a problem hiding this comment.
It's still not super clear to me what's the mapping between Memo content and which callbacks are executed. Does anyone have a pointer to learn this better?
There was a problem hiding this comment.
the main thing is that the special keys in the callback data specify on which chain the callback should be executed.
Which is why that in this test case, it is only being executed on the dest chain. and not the src (which would be chain B in this case)
bznein
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk and
chatton
as code owners
| { | ||
| name: "ack callback", | ||
| testMemo: fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract), | ||
| expCallbackMapOnChainB: map[callbacktypes.CallbackType]int{callbacktypes.CallbackTypeAcknowledgementPacket: 1}, | ||
| expCallbackMapOnChainC: map[callbacktypes.CallbackType]int{}, | ||
| }, |
There was a problem hiding this comment.
might be worth adding a memo that contains both src_callback and dest_callback, in the happy path I believe we should have
expCallbackMapOnChainB: map[callbacktypes.CallbackType]int{callbacktypes.CallbackTypeAcknowledgementPacket: 1}
expCallbackMapOnChainC: map[callbacktypes.CallbackType]int{callbacktypes.CallbackTypeReceivePacket: 1},
There was a problem hiding this comment.
{
name: "ack and recv callback",
testMemo: fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address": "%s"}}`, simapp.SuccessContract, simapp.SuccessContract),
expCallbackMapOnChainB: map[callbacktypes.CallbackType]int{callbacktypes.CallbackTypeAcknowledgementPacket: 1},
expCallbackMapOnChainC: map[callbacktypes.CallbackType]int{callbacktypes.CallbackTypeReceivePacket: 1},
},There was a problem hiding this comment.
Good one thank you! added!
|
There was a problem hiding this comment.
Test code looks super clean, nice work.
I do find the flow a bit strange for invoking the source chain callbacks from an intermediary hop, but maybe that's fine. I think that should be documented and well understood tho.
I think it would be easy to assume "source chain callbacks on A" and "dest chain callbacks on C" as well
| testCases := []struct { | ||
| name string | ||
| testMemo string | ||
| expCallbackMapOnChainB map[callbacktypes.CallbackType]int |
There was a problem hiding this comment.
With the current implementation it makes sense that this would be the case.. but it is a little weird imo.
Wonder if we should have a testcase for ensuring that the send_packet callback is never invoked on chainA
| s.Require().NoError(err) | ||
|
|
||
| // We never expect chainA to have executed any callback | ||
| s.Require().Empty(GetSimApp(s.chainA).MockContractKeeper.Counters, "chainA's callbacks counter map is not empty") |
There was a problem hiding this comment.
maybe this covers my thoughts on any source chain callback being invoked on A!
* Add test for callbacks in forwarding scenario * Add comments * Lint * Added test case. (cherry picked from commit 98183d9)
…kport #6805) (#6884) * (test) Add tests for callback execution when forwarding a packet (#6805) * Add test for callbacks in forwarding scenario * Add comments * Lint * Added test case. (cherry picked from commit 98183d9) * Update forwarding_test.go --------- Co-authored-by: Nikolas De Giorgis <nikolas.degiorgis@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Description
ref: #6578
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.