Skip to content
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

Merged
merged 3 commits into from
May 10, 2023

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Apr 21, 2023

(cherry picked from commit 7cd5893)

# Conflicts:
#	x/wasm/ibc.go
#	x/wasm/ibc_test.go
#	x/wasm/keeper/relay.go
@mergify mergify bot requested a review from alpe as a code owner April 21, 2023 16:36
@mergify mergify bot added the conflicts label Apr 21, 2023
@alpe alpe requested review from pinosu and webmaster128 and removed request for alpe April 24, 2023 07:00
@alpe alpe removed the conflicts label Apr 24, 2023
Copy link
Member

@webmaster128 webmaster128 left a 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

x/wasm/keeper/relay.go Show resolved Hide resolved
@alpe alpe requested a review from ethanfrey April 28, 2023 12:25
x/wasm/keeper/relay.go Outdated Show resolved Hide resolved
Copy link
Member

@ethanfrey ethanfrey left a 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

x/wasm/keeper/relay_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/relay_test.go Show resolved Hide resolved
},
"consume gas on error, ignore events + messages": {
"contract Err result converted to error Ack": {
Copy link
Member

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
Copy link
Member

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)

Copy link
Contributor

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

Copy link
Member

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

Copy link
Contributor

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

x/wasm/ibc.go Outdated Show resolved Hide resolved
expAck: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
expEvents: sdk.Events{
{
Type: "ibc_packet_received",
Copy link
Member

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

x/wasm/ibc_test.go Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member

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:

  1. panic in wasm contract triggers panic in keeper (using real wasmvm handler)
  2. contract error not redacted in returned ack packet (using ibctesting, passing through full ibc-go stack)
  3. error in submessage (eg invalid staking message) is redacted in returned ack packet.

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
@alpe
Copy link
Contributor

alpe commented May 8, 2023

I have added an integration test with a real contract to verify the full stack

@alpe alpe merged commit 70f077d into releases/v0.3x May 10, 2023
@alpe alpe deleted the mergify/bp/releases/v0.3x/pr-1353 branch May 10, 2023 07:46
@alpe alpe restored the mergify/bp/releases/v0.3x/pr-1353 branch May 10, 2023 07:51
@alpe alpe deleted the mergify/bp/releases/v0.3x/pr-1353 branch May 10, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants