Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(baseapp): correctly check errors before sealing in BaseApp.Init #18727

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

odeke-em
Copy link
Collaborator

This change ensures that we correctly check if the app has a nil commit multistore before trying to dereference it or invoking .Seal()

Fixes #18726

@odeke-em odeke-em requested a review from a team as a code owner December 13, 2023 04:46
Copy link
Contributor

coderabbitai bot commented Dec 13, 2023

Walkthrough

The changes involve a critical bug fix in the BaseApp.Init method of a software application. The fix addresses an issue where the method could panic due to nil dereferencing. The update ensures that the method now returns errors from a nil commit multistore before the application is sealed, improving error handling during the initialization phase.

Changes

File Path Change Summary
CHANGELOG.md Bug fix documented related to BaseApp.Init method to prevent nil dereferencing panics.
baseapp/baseapp.go Modified Init method to check for app.cms being nil and return errors before sealing the app.
baseapp/regression_test.go Added a new test TestNilCmsCheckBeforeSeal to ensure proper error handling when app.cms is nil.

Assessment against linked issues

Objective Addressed Explanation
Identify and resolve the contradiction in the code where app.cms is checked for nil after operations that could lead to nil pointer panics or prevent progress (Issue #18726)
Propose and implement a change to move the check for app.cms == nil before operations like app.setState to avoid potential nil pointer panics (Issue #18726)
Ensure meaningful progress by adjusting the order of operations related to app.cms initialization and usage, even if app.Seal() is invoked before setting cms (Issue #18726)

The code changes have been assessed against the objectives from the linked issues, and they have been addressed successfully without any need for further explanation. The changes ensure that the BaseApp.Init method now properly handles a nil commit multistore and prevents the application from being sealed in an erroneous state.

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 X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@odeke-em odeke-em force-pushed the baseapp-check-nil-cms-before-sealing branch 7 times, most recently from a5ac349 to df7f971 Compare December 13, 2023 08:43
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

CHANGELOG.md Outdated Show resolved Hide resolved
This change ensures that we correctly check if the app has a nil
commit multistore before trying to dereference it or invoking .Seal()

Fixes #18726
@odeke-em odeke-em force-pushed the baseapp-check-nil-cms-before-sealing branch from df7f971 to 4f1bd6e Compare December 13, 2023 10:37
@odeke-em odeke-em added this pull request to the merge queue Dec 13, 2023
Merged via the queue into main with commit fe95384 Dec 13, 2023
57 of 58 checks passed
@odeke-em odeke-em deleted the baseapp-check-nil-cms-before-sealing branch December 13, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants