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

feat(runtime/v2): store loader on simappv2 #21704

Merged
merged 57 commits into from
Sep 13, 2024
Merged

feat(runtime/v2): store loader on simappv2 #21704

merged 57 commits into from
Sep 13, 2024

Conversation

randygrok
Copy link
Collaborator

@randygrok randygrok commented Sep 13, 2024

Description

Closes: #21637


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a flexible store loading mechanism, allowing for custom loading strategies.
    • Enhanced store upgrade capabilities for better version management.
    • Added a feature to simulate nested messages in the base application.
    • Added documentation clarifying the location of the StoreLoader helper function.
  • Bug Fixes

    • Improved the upgrade handling process to streamline store upgrades.
  • Documentation

    • Updated testing capabilities by adding new dependencies for enhanced functionality.
  • Refactor

    • Cleaned up code related to store loader configurations for clarity and relevance.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
runtime/v2/store.go (1)

69-90: LGTM with a suggestion!

The UpgradeStoreLoader function implementation is correct and provides a robust mechanism for handling store upgrades based on the provided upgradeHeight and storeUpgrades. It checks if the store supports upgrades and applies them accordingly, falling back to the default loading behavior if no upgrades are needed. The comments provide a clear explanation of the function's purpose and behavior.

To further improve the documentation, consider adding a godoc comment for the UpgradeStoreLoader function, providing a more detailed explanation of its parameters, return value, and usage. This will enhance the overall documentation quality and make it easier for other developers to understand and use this function.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 346e715 and 7586ee2.

Files selected for processing (4)
  • runtime/store.go (2 hunks)
  • runtime/v2/store.go (2 hunks)
  • simapp/v2/upgrades.go (2 hunks)
  • x/upgrade/types/storeloader.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • simapp/v2/upgrades.go
Additional context used
Path-based instructions (3)
x/upgrade/types/storeloader.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

runtime/v2/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

runtime/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (4)
x/upgrade/types/storeloader.go (1)

3-6: Refactoring Approved: Simplifies Code and Maintains Backward Compatibility

The refactoring of moving the UpgradeStoreLoader function to the runtime package and assigning its value to a variable for backward compatibility is a good change because:

  1. It simplifies the code by delegating the store loading logic to the runtime package's implementation, promoting a more modular design.
  2. The variable assignment ensures that existing functionality remains accessible, maintaining backward compatibility.
  3. The change aligns with the objective of enabling more flexible data loading and upgrade capabilities.

Great work on this refactoring!

runtime/v2/store.go (2)

60-62: LGTM!

The StoreLoader type definition is correct and the comment clearly explains its purpose of enabling custom store loading mechanisms, which is useful for store upgrades.


64-67: LGTM!

The DefaultStoreLoader function implementation is correct and the comment clearly explains its behavior of calling LoadLatestVersion on the provided store.

runtime/store.go (1)

184-202: LGTM!

The UpgradeStoreLoader function is a well-designed addition that provides a flexible mechanism for custom store loading during upgrades. It takes the upgrade height and store upgrades as parameters and returns a baseapp.StoreLoader.

The function correctly checks if the current commit version matches the specified upgrade height and if there are any store upgrades to apply. If the conditions are met, it invokes LoadLatestVersionAndUpgrade on the CommitMultiStore with the provided upgrades. If the upgrade conditions are not met, it falls back to the default store loader using baseapp.DefaultStoreLoader.

The function signature and parameters follow the Golang conventions, and the code is properly documented with comments explaining its purpose and behavior.

Overall, this addition enhances the upgrade capabilities of the application by allowing custom store loading logic during upgrades.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm!

@julienrbrt julienrbrt added this pull request to the merge queue Sep 13, 2024
Merged via the queue into main with commit 7d6ff0d Sep 13, 2024
74 checks passed
@julienrbrt julienrbrt deleted the feat/store-loader branch September 13, 2024 14:27
mergify bot pushed a commit that referenced this pull request Sep 13, 2024
Co-authored-by: marbar3778 <[email protected]>
(cherry picked from commit 7d6ff0d)

# Conflicts:
#	CHANGELOG.md
#	runtime/v2/app.go
#	runtime/v2/module.go
#	runtime/v2/store.go
julienrbrt added a commit that referenced this pull request Sep 13, 2024
alpe added a commit that referenced this pull request Sep 16, 2024
* main:
  test: fix sims (#21735)
  build: bump proto-builder (#21730)
  refactor(schema)!: rename IntegerStringKind and DecimalStringKind (#21694)
  feat(types/collections): add `LegacyDec` collection value (#21693)
  refactor(server): alias AppOptions to coreserver.DynamicConfig (#21711)
  refactor(simapp): simplify simapp di (#21718)
  feat: replace the cosmos-db usecases in the tests with `core/testing` (#21525)
  feat(runtime/v2): store loader on simappv2 (#21704)
  docs(x/auth): vesting (#21715)
  build(deps): Bump google.golang.org/grpc from 1.66.1 to 1.66.2 (#21670)
  refactor(systemtest): Add cli.RunAndWait for common operations (#21689)
  fix(runtime/v2): provide default factory options if unset in app builder (#21690)
  chore: remove duplicate proto files for the same proto file (#21648)
  feat(x/genutil): add better error messages for genesis validation (#21701)
  build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.1 to 1.0.0-alpha.2 (#21698)
  test: migrate e2e/bank to system tests (#21607)
  chore: fix the gci lint issue in testutil (#21695)
  docs(x/authz): update grant docs (#21677)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: set StoreLoader in v2 app
4 participants