test: added packet data forwards compatibility test #6089
Conversation
srdtrk
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThe update includes a new test function, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/transfer/keeper/relay_test.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
modules/apps/transfer/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (1)
modules/apps/transfer/keeper/relay_test.go (1)
917-999: Consider adding negative test cases forTestPacketBackwardsCompatibilityto cover scenarios where the new field disrupts the expected packet processing.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/transfer/keeper/relay_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/keeper/relay_test.go
| // We are testing a scenario where a packet in the future has a new populated | ||
| // field called "new_field". And this packet is being sent to this module which | ||
| // doesn't have this field in the packet data. The module should be able to handle | ||
| // this packet without any issues. |
There was a problem hiding this comment.
this test will test is effectively testing compatibility of new fields in the packet data in version N and version N+1 right? If we were to run an E2E and add a new field to packet data and execute a MsgTransfer against an older version of ibc-go (pre-json marshalling) this could still blow up right?
There was a problem hiding this comment.
actually I guess it just means that whichever version we are about to tag will be able to successfully process unknown fields, it doesn't necessarily mean it will be backwards compatible with a previous release right?
There was a problem hiding this comment.
Yes. Unless we backport this feature
There was a problem hiding this comment.
Maybe this should be called a future proof test?
|
colin-axner
changed the title
|
Description
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit