fix: no-op rather than panic in solomachine update state #6313
Conversation
WalkthroughWalkthroughThe recent updates to the SoloMachine light client module improve error handling and stability. The changes ensure that invalid misbehaviour submissions no longer cause panics by returning an empty slice instead. Additionally, test cases have been updated to reflect these changes and handle new conditional checks related to consensus heights. 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 (
|
damiannolan
requested review from
colin-axner,
AdityaSripal and
chatton
as code owners
![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
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/light-clients/06-solomachine/light_client_module_test.go (2 hunks)
- modules/light-clients/06-solomachine/update.go (2 hunks)
Additional Context Used
Path-based Instructions (2)
modules/light-clients/06-solomachine/update.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/06-solomachine/light_client_module_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 (2)
modules/light-clients/06-solomachine/light_client_module_test.go (2)
1045-1051: The test case for invalid misbehaviour has been updated to no longer expect a panic. Ensure that this change aligns with the updated behavior in theUpdateStatefunction, which now performs a no-op instead of panicking.Verify that the
UpdateStatefunction has been updated to handle invalid misbehaviour types by performing a no-op instead of panicking.
1095-1099: The new conditional check for the length ofconsensusHeightsensures that thenewClientStateis only updated ifconsensusHeightsis not empty. This change improves the robustness of the test by handling edge cases whereconsensusHeightsmight be empty.This change looks good and aligns with the updated behavior in the
UpdateStatefunction.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/light-clients/06-solomachine/update.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/06-solomachine/update.go
crodriguezvega
requested review from
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
176-176: Convert the bare URL to a markdown link for better readability.- Ref: https://keepachangelog.com/en/1.0.0/ + [Ref](https://keepachangelog.com/en/1.0.0/)
Line range hint
191-191: Remove the trailing space for consistency.- * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. + * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
CHANGELOG.md (1)
59-59: The changelog entry is well-formatted and clearly describes the change.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
177-177: Replace bare URL with a markdown link for better readability.- * [\#3636](https://github.com/cosmos/ibc-go/issues/3636) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. + * [\#3636](https://github.com/cosmos/ibc-go/issues/3636) [Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions.](https://github.com/cosmos/ibc-go/issues/3636)
Line range hint
192-192: Remove trailing spaces to maintain clean and professional documentation.- * (light-clients/07-tendermint) [\#6276](https://github.com/cosmos/ibc-go/pull/6276) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. + * (light-clients/07-tendermint) [\#6276](https://github.com/cosmos/ibc-go/pull/6276) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. - * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. + * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions.Also applies to: 242-242
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
CHANGELOG.md (1)
60-60: Ensure the changelog entry for PR #6313 is accurate and complete.Verification successful
The changelog entry for PR #6313 appears to be accurate and complete based on the PR description and the changes made in the codebase. The entry correctly reflects the fix to avoid panicking on
UpdateStatefor invalid misbehaviour submissions in the SoloMachine light client.
- The PR title and description confirm the nature of the fix.
- The files changed in the PR (
CHANGELOG.md,light_client_module_test.go, andupdate.go) align with the changelog entry.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changelog entry for PR #6313 matches the PR description and changes. # Test: Search for the PR in the changelog. Expect: Entry matches PR description and changes. grep -A 5 -B 5 "#6313" CHANGELOG.mdLength of output: 976
Script:
#!/bin/bash # Description: Fetch the PR description and changes for PR #6313 to verify the changelog entry. # Fetch the PR description using GitHub CLI gh pr view 6313 --json title,body,files --jq '.title, .body, .files[].path'Length of output: 3415
|
Description
ref: #3636
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
consensusHeightsmore robustly in state updates.Documentation
CHANGELOG.mdto reflect the fix in the SoloMachine light client module regarding error handling improvements.