-
Notifications
You must be signed in to change notification settings - Fork 404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign IBC on packet recv error/ result.Err handling (backport #1353) #1358
Conversation
(cherry picked from commit 7cd5893) # Conflicts: # x/wasm/ibc.go # x/wasm/ibc_test.go # x/wasm/keeper/relay.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as I understand that matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides some cleanup and avoiding any event emitting on errors, I am not sure how this PR addresses the issue.
Basically, I think execErr != nil
=> panic is the desired behavior described in the issue and it was like that before this PR. I just want to make sure that case is triggered when the contract calls panic!()
in the wasm code, which would be best with a full-stack (non-mock) test using the hackatom contract, which has an entry point to trigger a panic
}, | ||
"consume gas on error, ignore events + messages": { | ||
"contract Err result converted to error Ack": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about emitting events and asserting they are not returned?
for i, m := range spec.contractResp.Messages { | ||
assert.Equal(t, (*capturedMsgs)[i], m.Msg) | ||
|
||
// verify msgs dispatched on success/ err response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, if you assert this here, you just need to return non-empty messages and events more often above to make this actually meaningful (with 0 sent, it passes in both cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IBCReceiveResult
contains either the Err
without events or an Ok
element which can have events. Submessages may have emitted events before, though.
The "event/error" logic is not in the relayer but downstream in our ibc.go
and tested in TestOnRecvPacket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct.
But most test cases don't dispatch any events, so these cases are not properly tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ibc_tests
the event handling fully covered. For all cases in TestOnRecvPacket
a custom event is emitted so that the behaviour can be verified. Only for success or nil Ack the myCustomEvent
is in the expected event set.
It can be confusing as we always have the ibc_packet_received
event for non panic cases
expAck: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")), | ||
expEvents: sdk.Events{ | ||
{ | ||
Type: "ibc_packet_received", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still surprised ibc-go module doesn't do this for us
It seems #1288 and #1289 are properly solved, but again a full-stack test showing how this is mangled (or not) via the ibc-go stack would be useful. The tests using mocks show the logic works as expected, but that expected comes with assumptions of how other components of the system work. It would be good to ensure that with at least:
The last 2 can use mock contract handler, but need to pass through the full stack. I didn't see such higher level tests |
* Apply review feedback * Better test name * Add contract integration test * Better doc in test and minor refactoring
I have added an integration test with a real contract to verify the full stack |
Backport of pull request #1353