Move IBC packet data interpretation to the application module #5890

michaelfig · 2020-03-30T04:09:00Z

Closes: #5870

Description

The main change is as the title: to move the IBC packet data interpretation to the application module to which it was routed. Notably, the TimeoutHeight is moved to PacketI and Data is a raw []byte again.

The tests pass for the entire x/ibc directory, but I'm unsure if I have covered all the corner cases.

I will need help from @jackzampolin and @cwgoes to get this over the finish line. Next comes testing in Agoric's Cosmos module.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

cwgoes · 2020-03-30T07:23:00Z

Hmm, I actually would prefer that we just get rid of channelexported.PacketDataI altogether and have all packets use an opaque data field, which the modules themselves must unmarshal. Is there a strong reason not to do that?

dtribble · 2020-03-30T14:40:13Z

I'm in favor of that as well. Would we still have the PacketDataI type as the thing that the golang modules unmarshal to?

michaelfig · 2020-03-30T15:11:29Z

Hmm, I actually would prefer that we just get rid of channelexported.PacketDataI altogether and have all packets use an opaque data field, which the modules themselves must unmarshal. Is there a strong reason not to do that?

No strong reason, so yes, I'll do that. I'm just getting my feet wet. 😄

Would we still have the PacketDataI type as the thing that the golang modules unmarshal to?

Probably an application-level interface (not exported by the channel) just to be able to specify the subset of codecs to use for the packet unmarshalling. I'll rename it just to prevent conflicts.

@cwgoes Should the IBC channel have a TimeoutHeight as part of the packet header (as it was before ADR-15)? It seems that this feature should be implemented in the channel, rather than the app module.

cwgoes · 2020-03-30T17:12:10Z

I'm in favor of that as well. Would we still have the PacketDataI type as the thing that the golang modules unmarshal to?

I don't think there needs to be a common interface if modules are decoding individually anyways.

No strong reason, so yes, I'll do that. I'm just getting my feet wet. smile

💯

Probably an application-level interface (not exported by the channel) just to be able to specify the subset of codecs to use for the packet unmarshalling. I'll rename it just to prevent conflicts.

Yes, exactly.

@cwgoes Should the IBC channel have a TimeoutHeight as part of the packet header (as it was before ADR-15)? It seems that this feature should be implemented in the channel, rather than the app module.

Yes, I think that makes sense, it should not be in each encoding separately for sure.

This also adds a channelexported.NewOpaquePacketData(rawBytes) implementation to assist apps in being able to manipulate the raw packet data using the codec layer.

codecov · 2020-03-31T01:06:12Z

Codecov Report

Merging #5890 into ibc-alpha will decrease coverage by <.01%.
The diff coverage is 43.47%.

@@              Coverage Diff              @@
##           ibc-alpha    #5890      +/-   ##
=============================================
- Coverage      35.25%   35.25%   -0.01%     
=============================================
  Files            408      408              
  Lines          42411    42409       -2     
=============================================
- Hits           14952    14950       -2     
  Misses         26108    26108              
  Partials        1351     1351

Impacted Files	Coverage Δ
x/ibc/04-channel/types/codec.go	`100% <ø> (ø)`	⬆️
x/ibc/04-channel/exported/exported.go	`93.44% <ø> (ø)`	⬆️
x/ibc/20-transfer/keeper/keeper.go	`50% <0%> (ø)`	⬆️
x/ibc/20-transfer/handler.go	`35.59% <0%> (+1.16%)`	⬆️
x/ibc/04-channel/keeper/packet.go	`84.23% <100%> (-0.99%)`	⬇️
x/ibc/20-transfer/keeper/relay.go	`97.95% <100%> (+0.02%)`	⬆️
x/ibc/04-channel/types/packet.go	`66.66% <27.27%> (-0.65%)`	⬇️
x/ibc/04-channel/types/msgs.go	`76.37% <45.45%> (-1.7%)`	⬇️
x/ibc/20-transfer/types/packet.go	`51.61% <50%> (+6.87%)`	⬆️
x/ibc/04-channel/keeper/timeout.go	`78.57% <50%> (ø)`	⬆️
... and 1 more

This one only has a GetBytes() method, which is implemented by OpaquePacketData.

cwgoes

Thanks, this is exactly the right direction, I think we can fail even faster on the deserialisation.

docs/architecture/adr-015-ibc-packet-receiver.md

x/ibc/04-channel/exported/opaque.go

x/ibc/20-transfer/keeper/keeper.go

No need to wrap the []byte packet.GetData(). If the caller wants it, they can use it directly.

cwgoes

utACK

docs/architecture/adr-015-ibc-packet-receiver.md

x/ibc/04-channel/exported/exported.go

AdityaSripal

Great improvement! Main request is that the packet timeout is still committed correctly with these new changes

x/ibc/20-transfer/types/codec.go

x/ibc/20-transfer/keeper/keeper.go

x/ibc/04-channel/types/packet.go

fedekunze

LGTM. Pending @AdityaSripal's comments to be resolved

x/ibc/04-channel/types/msgs.go

x/ibc/20-transfer/keeper/keeper.go

jackzampolin · 2020-03-31T23:00:46Z

Should we implement MsgSendPacketData as a generic msg and have MsgTransfer be a wrapper around MsgSendPacketData

x/ibc/20-transfer/handler.go

fedekunze

utACK

fedekunze

one minor improvement

x/ibc/04-channel/types/packet.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

michaelfig · 2020-04-01T14:00:29Z

@jackzampolin writes:

Should we implement MsgSendPacketData as a generic msg and have MsgTransfer be a wrapper around MsgSendPacketData

That's a terrific idea! Doing this would greatly simplify the ICS-20 implementation and make it much more understandable in intent. Do you want to take this to an issue?

jackzampolin · 2020-04-01T14:01:30Z

Would like to see it impl in this PR tbh

fedekunze · 2020-04-01T14:14:40Z

Should we implement MsgSendPacketData as a generic msg and have MsgTransfer be a wrapper around MsgSendPacketData

This is a great idea but imo out of scope for this PR. If we get this in I can reduce the number of changes to the proto migration and I prefer not to be blocked 😄

AdityaSripal

utACK 🎉

x/ibc/04-channel/types/packet.go

michaelfig requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners March 30, 2020 04:09

michaelfig force-pushed the mfig/dibc-experiment branch from 09dc1a8 to 9ef298b Compare March 30, 2020 04:11

cwgoes self-requested a review March 30, 2020 06:46

fix: move IBC packet data interpretation to the application module

b5070a4

This also adds a channelexported.NewOpaquePacketData(rawBytes) implementation to assist apps in being able to manipulate the raw packet data using the codec layer.

michaelfig force-pushed the mfig/dibc-experiment branch from 9ef298b to b5070a4 Compare March 31, 2020 01:06

feat: use an internal-to-module PacketDataI type

723704f

This one only has a GetBytes() method, which is implemented by OpaquePacketData.

michaelfig mentioned this pull request Mar 31, 2020

Go-to-JS layer dynamic IBC Agoric/agoric-sdk#817

Merged

cwgoes suggested changes Mar 31, 2020

View reviewed changes

docs/architecture/adr-015-ibc-packet-receiver.md Outdated Show resolved Hide resolved

x/ibc/04-channel/exported/opaque.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved

michaelfig added 2 commits March 31, 2020 09:22

fix: remove OpaquePacketData

92038fd

No need to wrap the []byte packet.GetData(). If the caller wants it, they can use it directly.

docs: update adr-015

1000af9

michaelfig requested a review from cwgoes March 31, 2020 15:30

cwgoes approved these changes Mar 31, 2020

View reviewed changes

cwgoes requested a review from AdityaSripal March 31, 2020 18:18

jackzampolin reviewed Mar 31, 2020

View reviewed changes

docs/architecture/adr-015-ibc-packet-receiver.md Show resolved Hide resolved

jackzampolin reviewed Mar 31, 2020

View reviewed changes

docs/architecture/adr-015-ibc-packet-receiver.md Outdated Show resolved Hide resolved

jackzampolin reviewed Mar 31, 2020

View reviewed changes

x/ibc/04-channel/exported/exported.go Show resolved Hide resolved

jackzampolin reviewed Mar 31, 2020

View reviewed changes

x/ibc/04-channel/exported/exported.go Show resolved Hide resolved

AdityaSripal requested changes Mar 31, 2020

View reviewed changes

x/ibc/20-transfer/types/codec.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved

x/ibc/04-channel/types/packet.go Outdated Show resolved Hide resolved

fedekunze reviewed Mar 31, 2020

View reviewed changes

x/ibc/04-channel/types/msgs.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved

fix: put the TimeoutHeight back into the packet commitment

36d8c43

michaelfig added 2 commits March 31, 2020 23:48

refactor: simplify unmarshalling of transfer packet

f0cba60

docs: update for new unmarshal steps

40e631f

golangcibot reviewed Apr 1, 2020

View reviewed changes

x/ibc/20-transfer/handler.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/handler.go Outdated Show resolved Hide resolved

fix: clean up usage of sdkerrors.Wrapf

754b4fa

michaelfig requested review from AdityaSripal and fedekunze April 1, 2020 06:41

fedekunze approved these changes Apr 1, 2020

View reviewed changes

fedekunze reviewed Apr 1, 2020

View reviewed changes

x/ibc/04-channel/types/packet.go Outdated Show resolved Hide resolved

x/ibc/04-channel/types/packet.go Outdated Show resolved Hide resolved

x/ibc/04-channel/types/packet.go Show resolved Hide resolved

Apply suggestions from code review

c6c0598

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

AdityaSripal approved these changes Apr 1, 2020

View reviewed changes

fedekunze reviewed Apr 1, 2020

View reviewed changes

x/ibc/04-channel/types/packet.go Outdated Show resolved Hide resolved

chore: remove unnecessary "encoding/binary" import

842d73d

fedekunze added R4R labels Apr 1, 2020

fedekunze merged commit d456271 into cosmos:ibc-alpha Apr 1, 2020

michaelfig deleted the mfig/dibc-experiment branch April 1, 2020 19:45

cwgoes mentioned this pull request Apr 6, 2020

Change the encoding pipeline for IBC packet data #5870

Closed

Itchibon777 mentioned this pull request Oct 27, 2023

[Snyk] Security upgrade axios from 0.19.0 to 1.6.0 Itchibon777/cosmos-sdk#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move IBC packet data interpretation to the application module #5890

Move IBC packet data interpretation to the application module #5890

michaelfig commented Mar 30, 2020 •

edited

Loading

cwgoes commented Mar 30, 2020

dtribble commented Mar 30, 2020

michaelfig commented Mar 30, 2020

cwgoes commented Mar 30, 2020

codecov bot commented Mar 31, 2020 •

edited

Loading

cwgoes left a comment

cwgoes left a comment

AdityaSripal left a comment

fedekunze left a comment

jackzampolin commented Mar 31, 2020

fedekunze left a comment

fedekunze left a comment

michaelfig commented Apr 1, 2020

jackzampolin commented Apr 1, 2020

fedekunze commented Apr 1, 2020

AdityaSripal left a comment

Move IBC packet data interpretation to the application module #5890

Move IBC packet data interpretation to the application module #5890

Conversation

michaelfig commented Mar 30, 2020 • edited Loading

Description

cwgoes commented Mar 30, 2020

dtribble commented Mar 30, 2020

michaelfig commented Mar 30, 2020

cwgoes commented Mar 30, 2020

codecov bot commented Mar 31, 2020 • edited Loading

Codecov Report

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

jackzampolin commented Mar 31, 2020

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

michaelfig commented Apr 1, 2020

jackzampolin commented Apr 1, 2020

fedekunze commented Apr 1, 2020

AdityaSripal left a comment

Choose a reason for hiding this comment

michaelfig commented Mar 30, 2020 •

edited

Loading

codecov bot commented Mar 31, 2020 •

edited

Loading