-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor(x/upgrade): migrate upgrade to env var #19443
Conversation
WalkthroughWalkthroughThe recent updates to the upgrade module bring significant architectural changes aimed at simplifying integration and enhancing modularity by introducing 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are needed in app.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (9)
- x/upgrade/CHANGELOG.md (2 hunks)
- x/upgrade/depinject.go (3 hunks)
- x/upgrade/keeper/abci.go (3 hunks)
- x/upgrade/keeper/abci_test.go (7 hunks)
- x/upgrade/keeper/grpc_query_test.go (2 hunks)
- x/upgrade/keeper/keeper.go (16 hunks)
- x/upgrade/keeper/keeper_test.go (3 hunks)
- x/upgrade/keeper/migrations.go (2 hunks)
- x/upgrade/module.go (1 hunks)
Additional comments: 36
x/upgrade/depinject.go (1)
- 39-39: LGTM! The integration of
appmodule.Environment
inModuleInputs
aligns with the PR's objectives for enhanced configuration flexibility.x/upgrade/keeper/migrations.go (2)
- 32-32: LGTM! Accessing
KVStoreService
throughenvironment
inMigrate1to2
aligns with the refactor objectives.- 69-69: LGTM! Using
environment
to accessKVStoreService
inmigrateAppVersion
is consistent with the PR's goals.x/upgrade/CHANGELOG.md (1)
- 34-34: LGTM! The documentation accurately reflects the integration of
appmodule.Environment
and its implications.x/upgrade/keeper/abci.go (1)
- 28-28: LGTM! Using
k.environment.HeaderService.GetHeaderInfo(ctx).Height
for block height access aligns with the refactor objectives.x/upgrade/module.go (1)
- 155-155: LGTM! Simplifying the
PreBlock
method call to usePreBlocker
directly on thekeeper
field enhances readability and maintainability.x/upgrade/keeper/grpc_query_test.go (2)
- 41-41: LGTM! Initializing
env
withruntime.NewEnvironment
andlog.NewNopLogger()
aligns with the refactor objectives.- 49-49: LGTM! Updating the
keeper.NewKeeper
call to includeenv
as the first argument is consistent with the PR's goals.x/upgrade/keeper/keeper_test.go (4)
- 49-49: LGTM! Initializing
env
withruntime.NewEnvironment
andlog.NewNopLogger()
inSetupTest
aligns with the refactor objectives.- 72-72: LGTM! Updating the
keeper.NewKeeper
call inSetupTest
to includeenv
as the first argument is consistent with the PR's goals.- 256-257: LGTM! Initializing
env
withruntime.NewEnvironment
andlog.NewNopLogger()
inTestIsSkipHeight
aligns with the refactor objectives.- 257-257: LGTM! Updating the
keeper.NewKeeper
call inTestIsSkipHeight
to includeenv
as the first argument is consistent with the PR's goals.x/upgrade/keeper/keeper.go (19)
- 34-36: The addition of the
environment
field in theKeeper
struct aligns with the objective of utilizing environment variables for configuration flexibility.- 51-55: The
NewKeeper
function correctly accepts anappmodule.Environment
parameter and initializes theenvironment
field in theKeeper
struct.- 91-91: The use of
runtime.KVStoreAdapter
to adapt theKVStoreService
from theenvironment
for setting module version maps is appropriate and follows the intended design changes.- 118-118: Accessing the key-value store service through the
environment
for getting the module version map is correctly implemented.- 140-140: The method
GetModuleVersions
correctly utilizes theenvironment
to access the key-value store service for fetching module versions.- 165-165: The method
getModuleVersion
properly uses theenvironment
to access the key-value store service for retrieving a specific module version.- 195-195: In
ScheduleUpgrade
, the use ofenvironment.HeaderService.GetHeaderInfo
to get the current block height for validation is correctly implemented.- 208-208: Utilizing the
environment
to open a key-value store inScheduleUpgrade
for storing the upgrade plan is correctly done.- 241-241: The method
SetUpgradedClient
correctly uses theenvironment
to access the key-value store for setting the upgraded client.- 248-248: The method
GetUpgradedClient
properly uses theenvironment
to access the key-value store for retrieving the upgraded client.- 264-264: The method
SetUpgradedConsensusState
correctly uses theenvironment
to access the key-value store for setting the upgraded consensus state.- 271-271: The method
GetUpgradedConsensusState
properly uses theenvironment
to access the key-value store for retrieving the upgraded consensus state.- 286-286: The method
GetLastCompletedUpgrade
correctly uses theenvironment
to access the key-value store for retrieving the last completed upgrade information.- 321-321: The method
GetDoneHeight
properly uses theenvironment
to access the key-value store for retrieving the height at which a given upgrade was executed.- 342-342: The method
ClearIBCState
correctly uses theenvironment
to access the key-value store for clearing any planned IBC state.- 368-368: The method
ClearUpgradePlan
correctly uses theenvironment
to access the key-value store for clearing any scheduled upgrade plan.- 374-374: The method
Logger
correctly utilizes theenvironment
to obtain a module-specific logger.- 380-380: The method
GetUpgradePlan
properly uses theenvironment
to access the key-value store for retrieving the currently scheduled upgrade plan.- 400-400: The method
setDone
correctly uses theenvironment
to access the key-value store for marking an upgrade as done.x/upgrade/keeper/abci_test.go (5)
- 117-117: The introduction of the
env
parameter in thesetupTest
function correctly reflects the changes made to thekeeper
structure, ensuring the test setup aligns with the new design.- 132-132: The
NewKeeper
function call within thesetupTest
function correctly includes theenv
parameter, aligning with the modifications made to thekeeper
structure.- 464-464: The introduction of the
env
parameter in theTestDowngradeVerification
function correctly reflects the changes made to thekeeper
structure, ensuring the test setup aligns with the new design.- 473-473: The
NewKeeper
function call within theTestDowngradeVerification
function correctly includes theenv
parameter, aligning with the modifications made to thekeeper
structure.- 522-522: The
NewKeeper
function call within theTestDowngradeVerification
function, for the downgrade scenario, correctly includes theenv
parameter, aligning with the modifications made to thekeeper
structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/upgrade/depinject.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/upgrade/depinject.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! but simapp/app.go should still be updated with the new api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as julien's comment, otherwise lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
ref #19290
migrate upgrade module to use env variabel
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
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.
I have...
Summary by CodeRabbit
appmodule.Environment
for improved integration.Keeper
structure for better context management and environmental interactions.Keeper
structure and environmental parameters.