Refactor panic to use return error #6391
Conversation
chatton
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
chatton
changed the title
WalkthroughThe changes focus on improving error handling in the ICS20-V2 conversion logic within the IBCModule. Specifically, panics have been replaced with error returns to enhance stability and robustness. This involves updates to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant IBCModule
participant ConvertInternal
Client->>IBCModule: Call unmarshalPacketDataBytesToICS20V2(bz, ics20Version)
IBCModule->>ConvertInternal: Call PacketDataV1ToV2(datav1)
ConvertInternal-->>IBCModule: Return convertedData, error
IBCModule-->>Client: Return convertedData, error
Assessment against linked issues
Possibly related issues
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: 2
Outside diff range and nitpick comments (1)
modules/apps/transfer/internal/convert/convert.go (1)
14-16: Refactor to return errors instead of panicking enhances robustness and error traceability.Consider improving the error message to include more context about the validation failure.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/transfer/ibc_module.go (1 hunks)
- modules/apps/transfer/internal/convert/convert.go (3 hunks)
- modules/apps/transfer/internal/convert/convert_test.go (2 hunks)
Additional Context Used
Path-based Instructions (3)
modules/apps/transfer/internal/convert/convert.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/internal/convert/convert_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"modules/apps/transfer/ibc_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
modules/apps/transfer/ibc_module.go (1)
187-187: Refactor to handle different ICS20 versions and return errors enhances robustness.Ensure that all potential ICS20 versions are correctly handled to avoid runtime errors.
#!/bin/bash # Description: Verify all ICS20 versions are handled in `unmarshalPacketDataBytesToICS20V2`. # Test: Search for version handling in the function. Expect: Handling for all known versions. rg --type go $'unmarshalPacketDataBytesToICS20V2' modules/apps/transfer/ibc_module.go
| @@ -40,7 +44,7 @@ func ExtractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) { | |||
|
|
|||
| // this condition should never be reached. | |||
| if len(splitPath)%2 != 0 { | |||
| panic("pathSlice length is not even") | |||
| panic(fmt.Errorf("path slice length is not even")) | |||
There was a problem hiding this comment.
Consider replacing panic with error handling to maintain consistency in error handling across the codebase.
| @@ -20,7 +20,7 @@ func TestConvertPacketV1ToPacketV2(t *testing.T) { | |||
| name string | |||
| v1Data types.FungibleTokenPacketData | |||
| v2Data types.FungibleTokenPacketDataV2 | |||
| expPanic error | |||
| expError error | |||
There was a problem hiding this comment.
Updated test cases to reflect new error handling in PacketDataV1ToV2.
Consider adding more edge cases to ensure comprehensive coverage.
Would you like assistance in identifying potential edge cases for additional tests?
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/transfer/ibc_module.go (1 hunks)
- modules/apps/transfer/internal/convert/convert.go (3 hunks)
- modules/apps/transfer/internal/convert/convert_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- modules/apps/transfer/ibc_module.go
- modules/apps/transfer/internal/convert/convert.go
- modules/apps/transfer/internal/convert/convert_test.go
|
chatton
deleted the
cian/issue#6387-clean-up-panics-in-ics20v2-conversion-logic
branch
Description
closes: #6387
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
Bug Fixes
Refactor
Tests