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.

Sign up for GitHub

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

Return IBC packet sequence number #1225

Merged
merged 4 commits into from
Mar 6, 2023
Merged

Return IBC packet sequence number #1225

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb40d0b
Return IBC packet sequence number
pinosu Mar 2, 2023
9ad2807
Fix review feedbacks
pinosu Mar 3, 2023
819d945
Remove names to return values in DispatchMsg method
pinosu Mar 3, 2023
c6d5828
Fix comments
pinosu Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion docs/proto/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
- [cosmwasm/wasm/v1/ibc.proto](#cosmwasm/wasm/v1/ibc.proto)
- [MsgIBCCloseChannel](#cosmwasm.wasm.v1.MsgIBCCloseChannel)
- [MsgIBCSend](#cosmwasm.wasm.v1.MsgIBCSend)
- [MsgIBCSendResponse](#cosmwasm.wasm.v1.MsgIBCSendResponse)

- [cosmwasm/wasm/v1/proposal.proto](#cosmwasm/wasm/v1/proposal.proto)
- [AccessConfigUpdate](#cosmwasm.wasm.v1.AccessConfigUpdate)
Expand Down Expand Up @@ -573,6 +574,21 @@ MsgIBCSend




<a name="cosmwasm.wasm.v1.MsgIBCSendResponse"></a>

### MsgIBCSendResponse
MsgIBCSendResponse


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `sequence` | [uint64](#uint64) | | sequence number of the transfer packet sent |





<!-- end messages -->

<!-- end enums -->
Expand Down Expand Up @@ -1457,7 +1473,7 @@ MsgStoreCode submit Wasm code to the system

| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `sender` | [string](#string) | | Sender is the that actor that signed the messages |
| `sender` | [string](#string) | | Sender is the actor that signed the messages |
| `wasm_byte_code` | [bytes](#bytes) | | WASMByteCode can be raw or gzip compressed |
| `instantiate_permission` | [AccessConfig](#cosmwasm.wasm.v1.AccessConfig) | | InstantiatePermission access control to apply on contract creation, optional |

Expand Down
6 changes: 6 additions & 0 deletions proto/cosmwasm/wasm/v1/ibc.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,9 @@ message MsgIBCSend {
message MsgIBCCloseChannel {
string channel = 2 [ (gogoproto.moretags) = "yaml:\"source_channel\"" ];
}

// MsgIBCSendResponse
message MsgIBCSendResponse {
// sequence number of the transfer packet sent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// sequence number of the transfer packet sent
// Sequence number of the IBC packet sent

uint64 sequence = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proto/cosmwasm/wasm/v1/tx.proto is not super consistent but usually we have the response type right after the message type. I think that's helpful for navigating. I'd move it above message MsgIBCCloseChannel {.

14 changes: 13 additions & 1 deletion x/wasm/keeper/handler_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,19 @@ func (h IBCRawPacketHandler) DispatchMsg(ctx sdk.Context, _ sdk.AccAddress, cont
ConvertWasmIBCTimeoutHeightToCosmosHeight(msg.IBC.SendPacket.Timeout.Block),
msg.IBC.SendPacket.Timeout.Timestamp,
)
return nil, nil, h.channelKeeper.SendPacket(ctx, channelCap, packet)

if err := h.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil {
return nil, nil, sdkerrors.Wrap(err, "failed to send packet")
}

resp := &types.MsgIBCSendResponse{Sequence: sequence}
val, err := resp.Marshal()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does work correct!
I had expected to see the codec.Codec passed to the IBCRawPacketHandler object as an abstraction layer but this is fine.

if err != nil {
return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")
return nil, nil, sdkerrors.Wrap(err, "failed to marshal IBC send response")

}

data = append(data, val)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to append. Just set the value here

Suggested change
data = append(data, val)
data = val

or better

return nil, val, nil

return nil, data, nil
}

var _ Messenger = MessageHandlerFunc(nil)
Expand Down
10 changes: 8 additions & 2 deletions x/wasm/keeper/handler_plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,14 +308,20 @@ func TestIBCRawPacketHandler(t *testing.T) {
capturedPacket = nil
// when
h := NewIBCRawPacketHandler(spec.chanKeeper, spec.capKeeper)
data, evts, gotErr := h.DispatchMsg(ctx, RandomAccountAddress(t), ibcPort, wasmvmtypes.CosmosMsg{IBC: &wasmvmtypes.IBCMsg{SendPacket: &spec.srcMsg}})
evts, data, gotErr := h.DispatchMsg(ctx, RandomAccountAddress(t), ibcPort, wasmvmtypes.CosmosMsg{IBC: &wasmvmtypes.IBCMsg{SendPacket: &spec.srcMsg}})
// then
require.True(t, spec.expErr.Is(gotErr), "exp %v but got %#+v", spec.expErr, gotErr)
if spec.expErr != nil {
return
}
assert.Nil(t, data)

assert.Nil(t, evts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? The events are nil? How do we get the ibc packet events to the relayers here?
(I guess they just pass through context and not via the return value?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, via context.

require.NotNil(t, data)

msg := &types.MsgIBCSendResponse{Sequence: 1}
val, err := msg.Marshal()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same code as used in the handler. When you unmarshal the returned data instead and compare the seq, you are acting as the client. This gives confidence that both sides work.

require.NoError(t, err)
assert.Equal(t, data[0], val)
assert.Equal(t, spec.expPacketSent, capturedPacket)
})
}
Expand Down
197 changes: 177 additions & 20 deletions x/wasm/types/ibc.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion x/wasm/types/tx.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.