feat: add v2.0 upgrade handler #75
Conversation
WalkthroughThe recent changes significantly enhance the upgrade management system within the application, transitioning from a static list of upgrade plans to a dynamic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UpgradeRouter
participant AppKeepers
participant AgentKeeper
User->>UpgradeRouter: Initiate Upgrade
UpgradeRouter->>AppKeepers: Retrieve Current State
AppKeepers->>AgentKeeper: Migrate Agents
AgentKeeper-->>AppKeepers: Confirm Migration
AppKeepers-->>UpgradeRouter: Migration Complete
UpgradeRouter-->>User: Upgrade Successful
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 Configuration 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: 4
Outside diff range, codebase verification and nitpick comments (1)
app/upgrades/v200/migarations.go (1)
13-28: Consider adding logging for better traceability.Adding logging can help trace the migration process and identify issues.
+ log.Printf("Migrating receiver: %s", receiver.Name)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/upgrade.go (2 hunks)
- app/upgrades/types.go (3 hunks)
- app/upgrades/v200/migarations.go (1 hunks)
- app/upgrades/v200/upgrades.go (1 hunks)
- x/agent/keeper/agent.go (1 hunks)
- x/agent/types/errors.go (1 hunks)
Additional comments not posted (22)
app/upgrades/v200/migarations.go (1)
23-24: Verify the removal of receivers.The TODO comment indicates uncertainty about the removal of
params.Receiversfrom the BTC staking module.Ensure that the
Receiversfield is correctly removed from the BTC staking module parameters.x/agent/types/errors.go (1)
15-15: LGTM! The new error variable is consistent and enhances error handling.The new error variable
ErrAdminExistsis correctly registered and follows the existing pattern.app/upgrade.go (3)
14-17: LGTM! The initialization block is correct.The initialization block correctly registers the v2.0 upgrade plan with the
UpgradeRouter.
20-22: LGTM! The method promotes a clearer logic flow.The
RegisterUpgradePlansmethod correctly callssetupUpgradeStoreLoadersandsetupUpgradeHandlers.
57-67: LGTM! The method aligns with the new dynamic routing system.The
setupUpgradeHandlersmethod correctly sets up upgrade handlers using theUpgradeRouter.app/upgrades/v200/upgrades.go (8)
1-15: LGTM! The package declaration and import statements are correctly structured.The imports are relevant to the functionality and follow standard practices.
17-28: LGTM! The Upgrade struct is correctly defined and initialized.The struct initialization sets up the upgrade handler and store upgrades appropriately.
30-34: LGTM! The upgradeHandlerConstructor function signature is correct.The function parameters and return type are appropriate for an upgrade handler.
35-38: Ensure correct module versioning.The function sets the consensus version for the agent and plan modules. Verify that these versions are correct and necessary for the upgrade.
50-53: LGTM! The agent module initialization is correct.The function migrates agents from BTCStaking to the agent module and handles errors appropriately.
55-62: LGTM! The plan module initialization is correct.The function sets the plan parameters and handles errors appropriately.
64-66: LGTM! The token module initialization is correct.The function sets the token parameters using the default values.
68-69: LGTM! The function returns the result of running migrations.The function returns the updated version map and any errors from running migrations.
app/upgrades/types.go (6)
Line range hint
1-17:
LGTM! The package declaration and import statements are correctly structured.The imports are relevant to the functionality and follow standard practices.
43-54: LGTM! The AppKeepers struct is correctly updated.The struct definition includes all necessary keepers and follows standard practices.
59-61: LGTM! The UpgradeRouter struct is correctly defined.The struct definition is appropriate for managing upgrades.
63-65: LGTM! The NewUpgradeRouter function is correctly implemented.The function initializes a new UpgradeRouter instance.
75-77: LGTM! The Routers function is correctly implemented.The function returns the map of registered upgrades.
79-81: LGTM! The UpgradeInfo function is correctly implemented.The function returns the upgrade information for a given plan name.
x/agent/keeper/agent.go (3)
47-54: Ensure correct admin address validation.The SetAdmin method checks if the sender is the current admin. Verify that the error handling and admin setting logic are correct.
Line range hint
55-62:
LGTM! The GetAdmin method is correctly implemented.The method retrieves and returns the admin address from the store.
Line range hint
111-113:
LGTM! The setAdmin method is correctly implemented.The method sets the admin address in the store.
| app.AgentKeeper.AddAgent( | ||
| ctx, | ||
| receiver.Name, | ||
| receiver.Addr, receiver.EthAddr, | ||
| "", | ||
| "", | ||
| ) |
There was a problem hiding this comment.
Handle errors from AddAgent.
The function does not handle potential errors from AddAgent. This could lead to silent failures.
- app.AgentKeeper.AddAgent(
+ if err := app.AgentKeeper.AddAgent(
ctx,
receiver.Name,
receiver.Addr, receiver.EthAddr,
"",
"",
); err != nil {
return err
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.AgentKeeper.AddAgent( | |
| ctx, | |
| receiver.Name, | |
| receiver.Addr, receiver.EthAddr, | |
| "", | |
| "", | |
| ) | |
| if err := app.AgentKeeper.AddAgent( | |
| ctx, | |
| receiver.Name, | |
| receiver.Addr, receiver.EthAddr, | |
| "", | |
| "", | |
| ); err != nil { | |
| return err | |
| } |
| // configure store loader that checks if version == upgradeHeight and applies store upgrades | ||
| func (app *LorenzoApp) setupUpgradeStoreLoaders() { | ||
| upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk() | ||
| if err != nil { | ||
| app.Logger().Info("not found upgrade plan", "planName", planName, "err", err.Error()) | ||
| panic(fmt.Sprintf("failed to read upgrade info from disk %s", err)) | ||
| } | ||
|
|
||
| if app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| return | ||
| } | ||
|
|
||
| if upgradeInfo.Name == planName && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| // this configures a no-op upgrade handler for the planName upgrade | ||
| app.UpgradeKeeper.SetUpgradeHandler(planName, upgradeHandler) | ||
| // configure store loader that checks if version+1 == upgradeHeight and applies store upgrades | ||
| app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, upgrades)) | ||
| app.SetStoreLoader( | ||
| upgradetypes.UpgradeStoreLoader( | ||
| upgradeInfo.Height, | ||
| router.UpgradeInfo(upgradeInfo.Name).StoreUpgrades, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Consider using a less aggressive error handling strategy.
Using panic for error handling might be too aggressive. Consider using error logging or returning an error.
- panic(fmt.Sprintf("failed to read upgrade info from disk %s", err))
+ log.Fatalf("failed to read upgrade info from disk %s", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // configure store loader that checks if version == upgradeHeight and applies store upgrades | |
| func (app *LorenzoApp) setupUpgradeStoreLoaders() { | |
| upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk() | |
| if err != nil { | |
| app.Logger().Info("not found upgrade plan", "planName", planName, "err", err.Error()) | |
| panic(fmt.Sprintf("failed to read upgrade info from disk %s", err)) | |
| } | |
| if app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | |
| return | |
| } | |
| if upgradeInfo.Name == planName && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | |
| // this configures a no-op upgrade handler for the planName upgrade | |
| app.UpgradeKeeper.SetUpgradeHandler(planName, upgradeHandler) | |
| // configure store loader that checks if version+1 == upgradeHeight and applies store upgrades | |
| app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, upgrades)) | |
| app.SetStoreLoader( | |
| upgradetypes.UpgradeStoreLoader( | |
| upgradeInfo.Height, | |
| router.UpgradeInfo(upgradeInfo.Name).StoreUpgrades, | |
| ), | |
| ) | |
| } | |
| // configure store loader that checks if version == upgradeHeight and applies store upgrades | |
| func (app *LorenzoApp) setupUpgradeStoreLoaders() { | |
| upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk() | |
| if err != nil { | |
| log.Fatalf("failed to read upgrade info from disk %s", err) | |
| } | |
| if app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | |
| return | |
| } | |
| app.SetStoreLoader( | |
| upgradetypes.UpgradeStoreLoader( | |
| upgradeInfo.Height, | |
| router.UpgradeInfo(upgradeInfo.Name).StoreUpgrades, | |
| ), | |
| ) | |
| } |
| func (r *UpgradeRouter) Register(u Upgrade) *UpgradeRouter { | ||
| if _, has := r.mu[u.UpgradeName]; has { | ||
| panic(u.UpgradeName + " already registered") | ||
| } | ||
| r.mu[u.UpgradeName] = u | ||
| return r | ||
| } |
There was a problem hiding this comment.
Handle duplicate registration appropriately.
The Register function panics if an upgrade is already registered. Consider handling this case more gracefully, such as logging an error or returning an error.
- panic(u.UpgradeName + " already registered")
+ return fmt.Errorf("%s already registered", u.UpgradeName)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *UpgradeRouter) Register(u Upgrade) *UpgradeRouter { | |
| if _, has := r.mu[u.UpgradeName]; has { | |
| panic(u.UpgradeName + " already registered") | |
| } | |
| r.mu[u.UpgradeName] = u | |
| return r | |
| } | |
| func (r *UpgradeRouter) Register(u Upgrade) *UpgradeRouter { | |
| if _, has := r.mu[u.UpgradeName]; has { | |
| return fmt.Errorf("%s already registered", u.UpgradeName) | |
| } | |
| r.mu[u.UpgradeName] = u | |
| return r | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/upgrades/v200/upgrades.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v200/upgrades.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/upgrades/v200/upgrades.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v200/upgrades.go
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
SetAdminmethod with its error handling.Chores