Hex Packet Data

[AdityaSripal]

Description

Encoding packet data in hex allows for more robust encoding/decoding for relayers. Proto-encoded packets were breaking with old bare string conversion. Raw binary can be passed through events using hex.

Tested live with go-relayer and two-chainz script

closes: #XXXX

---

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.

• 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
• Review Codecov Report in the comment section below once CI passes

[colin-axner]

I disagree with the proposed solution. Applications should hex encode before passing it to core IBC. We should check that the packet data is utf-8 in packet ValidateBasic, if anything

Edit: I changed my opinion, see below

[AdityaSripal]

cc relayer teams: @ethanfrey @ancazamfir

packet_data attribute is getting deprecated in favor of packet_data_hex. This will allow app developers to marshal their packet data with protobuf and relay without any issues

[AdityaSripal]

@colin-axner Why? That doesn't make any sense to me. If I want to protobuf encode my packet data i should just be able to do that.

With your proposal, I either shouldn't use protobuf. Or I need to do a secondary encoding to make it utf-8, and then decode twice in my application RecvPacket callback. This doesn't make sense.

Packet Data is raw data (marshaled in whatever preference of the app developer), it's not a string. It's up to us as core IBC to make sure that raw data gets to the other end correctly so that it can be unmarshaled as expected by app developer. It's not up to app developer to make their raw data look like a "string" for our convenience.

[colin-axner]

I gave it some more thought and this solution sounds good since it only changes how events are emitted.

If applications are using proto to marshal their packet data, then I don't see why other applications won't run into the same issue we are facing here. Is there usefulness in not hex encoding packet data before passing it to core IBC?

[colin-axner]

can you add a section to the migrations doc?

[AdityaSripal]

If applications are using proto to marshal their packet data, then I don't see why other applications won't run into the same issue we are facing here. Is there usefulness in not hex encoding packet data before passing it to core IBC?

This fix makes it such that applications that use proto to marshal won't face this issue at all. Assuming relayers upgrade to use the new attribute. So regardless of the encoding format that applications choose, IBC will work. As I tested, proto-marshalling transfer packet data works seamlessly if you encode/decode with hex. It breaks when you try to just do a raw conversion.

Without this fix, we are limiting application choice on encoding without a good reason. Note applications are returning a byte slice, not a string in packet.GetData. It's still incumbent on core IBC to turn that into a string to emit in events. And the obvious choice is to use a standardized encoding like hex rather than relying on string() and have applications run into this issue again.

[codecov-commenter]

Codecov Report

Merging #144 (40e9d5d) into main (4e145d8) will increase coverage by 13.64%.
The diff coverage is 87.77%.

@@             Coverage Diff             @@
##             main     #144       +/-   ##
===========================================
+ Coverage   65.22%   78.86%   +13.64%     
===========================================
  Files         127      109       -18     
  Lines        8413     6468     -1945     
===========================================
- Hits         5487     5101      -386     
+ Misses       2561     1008     -1553     
+ Partials      365      359        -6     

Impacted Files	Coverage Δ
...odules/light-clients/07-tendermint/types/update.go	77.89% <75.00%> (-0.78%)	⬇️
modules/light-clients/07-tendermint/types/store.go	88.88% <89.09%> (+0.31%)	⬆️
modules/core/04-channel/keeper/packet.go	93.02% <100.00%> (-2.28%)	⬇️
...light-clients/06-solomachine/types/client_state.go	66.33% <100.00%> (-4.03%)	⬇️
modules/core/03-connection/keeper/handshake.go	85.80% <0.00%> (-7.75%)	⬇️
modules/core/04-channel/keeper/keeper.go	90.27% <0.00%> (-1.09%)	⬇️
testing/simapp/state.go
testing/simapp/config.go
... and 9 more

[ethanfrey]

Thanks for the quick fix.