imp: Use json for marshalling/unmarshalling transfer packet data. #5778
Conversation
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan and
srdtrk
as code owners
DimitrisJim
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5778 +/- ##
==========================================
+ Coverage 81.59% 81.61% +0.02%
==========================================
Files 199 199
Lines 15167 15146 -21
==========================================
- Hits 12375 12362 -13
+ Misses 2326 2319 -7
+ Partials 466 465 -1
|
| // GetBytes is a helper for serialising | ||
| // GetBytes is a helper for serialising the packet to bytes. | ||
| // The memo field of FungibleTokenPacketData is marked with the JSON omitempty tag | ||
| // ensuring that the memo field is not included in the marshalled bytes if one is not specified. |
There was a problem hiding this comment.
though one thing I did notice is that Unmarshal seems to work just fine using an old type w/o memo defined even if marshalled bytes contain the memo, i.e:
// Check that we can unmarshal into another type that does not have memo defined
type PacketData struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
Sender string `json:"sender"`
Receiver string `json:"receiver"`
}
ftpdWithMemo = types.FungibleTokenPacketData{
Denom: denom,
Amount: amount,
Sender: sender,
Receiver: receiver,
Memo: "{memo: \"test\"}",
}
bz, err = json.Marshal(ftpdWithMemo)
suite.Require().NoError(err)
var packetData2 PacketData
err = json.Unmarshal(bz, &packetData2)
suite.Require().NoError(err)
no clue if this is the case with jsonpb tho
There was a problem hiding this comment.
must be an additional check done when marshaling protoJSON
| suite.Require().NoError(err) | ||
|
|
||
| // check that the memo field is not present in the marshalled bytes | ||
| suite.Require().NotContains(string(bz), "memo") |
There was a problem hiding this comment.
just to double check, if you did have the memo set to "abc", this would fail?
There was a problem hiding this comment.
yea! just added an additional commit to do that check.
|
We can open a pr to SDK to remove the |
yup! will work on having compat tests run on schedule soon, will run one before merging (later tonight) |
|
compatibility tests be green https://github.com/cosmos/ibc-go/actions/runs/7731257280 triggered manually based on my branch, release branch to test set to main based on workflow dispatch options |
Cleans up final usage of
MustSortJSON. Uses json for marshalling/unmarshallingFungibleTokenPacketData.closes: #3915
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.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.