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: upgrade handler for v1.1.0 #1706

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

k-yang
Copy link
Member

@k-yang k-yang commented Dec 13, 2023

Description

Fixes the UpgradeHandler for v1.1.0. The upgrade handler cannot be nil, or else the chain will panic.

panic: ApplyUpgrade should never be called without first checking HasHandler

Purpose

Upgrade mainnet with inflation

Summary by CodeRabbit

  • New Features

    • Implemented an upgrade handler for version 1.1.0 to enhance the application's functionality.
  • Bug Fixes

    • Resolved issues with the v1.1.0 upgrade handler to ensure smooth version transitions.
  • Documentation

    • Updated the CHANGELOG to reflect the latest modifications and improvements.
  • Refactor

    • Modified upgrade handling logic to improve maintainability and performance.

Copy link
Contributor

coderabbitai bot commented Dec 13, 2023

Walkthrough

The project has implemented an upgrade handler for version 1.1.0 and fixed issues related to it. The changes involve passing additional arguments to the CreateUpgradeHandler function, updating import statements, and modifying the Upgrade struct to handle new upgrade logic. Dependency updates and inflation allocation logic revisions are also part of the recent updates.

Changes

File(s) Summary
CHANGELOG.md Added upgrade handler for v1.1.0; fixed issues with v1.1.0 upgrade handler; updated dependencies and inflation allocation logic.
app/upgrades.go Modified setUpgradeHandlers to include app.mm and app.configurator as arguments to CreateUpgradeHandler.
app/upgrades/types.go Updated import to include github.com/cosmos/cosmos-sdk/types/module; changed CreateUpgradeHandler function signature.
app/upgrades/.../constants.go Altered Upgrade struct's CreateUpgradeHandler to return a new function signature handling upgrade logic.

Poem

🐇✨
In the code, we hop and leap,
Through versions, we take a giant sweep.
Handlers set, and upgrades flow,
Onward to v1.1.0, we go! 🚀🌟

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

@k-yang k-yang marked this pull request as ready for review December 13, 2023 19:37
@k-yang k-yang requested a review from a team as a code owner December 13, 2023 19:37
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 32de025 and e0c93a7.
Files selected for processing (4)
  • CHANGELOG.md (2 hunks)
  • app/upgrades.go (1 hunks)
  • app/upgrades/types.go (1 hunks)
  • app/upgrades/v1_1_0/constants.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • CHANGELOG.md
Additional comments: 4
app/upgrades.go (1)
  • 21-25: <!--Monologue:
    Thought 1: The rg command has successfully found references to CreateUpgradeHandler in the codebase, which confirms its existence.
    Reflect 1: The output shows that CreateUpgradeHandler is indeed present and used in app/upgrades.go, app/upgrades/types.go, and app/upgrades/v1_1_0/constants.go.

Thought 2: The output from app/upgrades/types.go indicates that CreateUpgradeHandler is defined as a function type that takes a *module.Manager and module.Configurator as parameters, which aligns with the PR's description.
Reflect 2: This confirms that the function signature has been updated to include the new parameters as intended.

Thought 3: The output from app/upgrades/v1_1_0/constants.go shows an actual function definition that matches the new signature, which is also consistent with the PR's objective.
Reflect 3: This suggests that the CreateUpgradeHandler function is properly defined to handle the upgrade process with the new parameters.

Thought 4: The output from app/upgrades.go shows a call to CreateUpgradeHandler with app.mm and app.configurator as arguments, which is exactly what was described in the PR summary and the AI-generated summary.
Reflect 4: This confirms that the call to CreateUpgradeHandler has been updated to pass the new arguments, and it is consistent with the changes described.

Thought 5: With the confirmation of the function's existence, its updated signature, and the updated call, the changes appear to be correctly implemented in the codebase.
Reflect 5: The verification process seems complete, and the changes align with the PR's objectives and the AI-generated summaries.
-->

The verification process has confirmed the presence and correct implementation of the CreateUpgradeHandler function in the codebase. The function signature has been updated to include the new parameters, and the calls to this function have been adjusted accordingly. The changes are consistent with the PR's objectives and the AI-generated summaries.

  • The function type is defined in app/upgrades/types.go.
  • The function definition with the new signature is present in app/upgrades/v1_1_0/constants.go.
  • The call to CreateUpgradeHandler with the new arguments is found in app/upgrades.go.
Analysis chain

The change to the setUpgradeHandlers function correctly adds app.mm and app.configurator as arguments to the CreateUpgradeHandler call. Ensure that the CreateUpgradeHandler function is defined to accept these new parameters and that all instances where this function is called have been updated accordingly.


The initial verification script did not produce any results, which suggests that the CreateUpgradeHandler function definition and calls might not match the expected patterns. To ensure that the function is defined with the new parameters and that all calls to it have been updated, I will generate a new script with broader search patterns.


The AST-based search scripts have not produced any results for the definition or calls to CreateUpgradeHandler. This suggests that the function may not exist in the codebase as expected, or there is a discrepancy that the AST search is not capturing. To confirm the presence of CreateUpgradeHandler and its usage, I will use rg (ripgrep) to perform a text-based search.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for the definition of `CreateUpgradeHandler` to ensure it matches the new signature.
ast-grep --lang go --pattern $'func ($_ *Upgrade) CreateUpgradeHandler($_ *module.Manager, $_ module.Configurator) $_ { $$$ }'

# Search for calls to `CreateUpgradeHandler` to ensure they all pass the correct arguments.
ast-grep --lang go --pattern $'($_ *$_).CreateUpgradeHandler($_, $_)'

Length of output: 198



Script:

#!/bin/bash
# Search for any definition of `CreateUpgradeHandler` to verify its existence and signature.
ast-grep --lang go --pattern $'func ($_ *Upgrade) CreateUpgradeHandler($$$) $_ { $$$ }'

# Search for any calls to `CreateUpgradeHandler` to verify that they are consistent with the new signature.
ast-grep --lang go --pattern $'($_ *$_).CreateUpgradeHandler($$$)'

Length of output: 156



Script:

#!/bin/bash
# Use ripgrep to search for the string "CreateUpgradeHandler" in the codebase.
rg "CreateUpgradeHandler"

Length of output: 388

app/upgrades/types.go (1)
  • 12-12: The signature of CreateUpgradeHandler has been updated to accept a *module.Manager and module.Configurator. Verify that all calls to this function throughout the codebase have been updated to match the new signature.
app/upgrades/v1_1_0/constants.go (2)
  • 17-20: The implementation of CreateUpgradeHandler now properly returns a non-nil UpgradeHandler function, which should prevent the panic described in the PR's objective. This function uses the RunMigrations method, which is presumably part of the module manager's responsibilities. Ensure that RunMigrations is designed to handle the upgrade process as expected and that the ctx, cfg, and fromVM parameters are correctly utilized within it.

  • 22-23: The addition of inflationtypes.ModuleName to the StoreUpgrades.Added slice appears to be related to the new inflation features. Confirm that this module is intended to be added during the upgrade process and that all necessary preparations for this module's inclusion (like migrations or initializations) are in place.

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Merging #1706 (d27703d) into main (b400eed) will increase coverage by 0.00%.
Report is 4 commits behind head on main.
The diff coverage is 77.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1706   +/-   ##
=======================================
  Coverage   73.93%   73.94%           
=======================================
  Files         191      191           
  Lines       15129    15125    -4     
=======================================
- Hits        11186    11184    -2     
+ Misses       3289     3287    -2     
  Partials      654      654           
Files Coverage Δ
app/upgrades.go 55.55% <100.00%> (ø)
wasmbinding/exec_perp.go 81.66% <100.00%> (+0.31%) ⬆️
x/perp/v2/keeper/dnr.go 85.15% <ø> (+0.74%) ⬆️
x/perp/v2/keeper/hooks.go 80.43% <100.00%> (ø)
x/perp/v2/keeper/keeper.go 97.46% <100.00%> (ø)
x/perp/v2/types/genesis.go 86.04% <100.00%> (+0.33%) ⬆️
x/perp/v2/types/market.go 100.00% <100.00%> (ø)
x/perp/v2/module/abci.go 65.00% <66.66%> (ø)
x/common/asset/pair.go 77.77% <0.00%> (-2.69%) ⬇️
x/perp/v2/client/cli/gen_market.go 84.00% <63.63%> (+0.90%) ⬆️

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e0c93a7 and d27703d.
Files selected for processing (1)
  • CHANGELOG.md (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

@Unique-Divine Unique-Divine merged commit a73dab4 into main Dec 13, 2023
17 checks passed
@Unique-Divine Unique-Divine deleted the fix/upgrade/v1.1.0-upgrade-handler branch December 13, 2023 22:02
k-yang added a commit that referenced this pull request Dec 14, 2023
* fix: upgrade handler for v1.1.0

* chore: update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants