-
Notifications
You must be signed in to change notification settings - Fork 235
Feat/upgrade #381
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/upgrade #381
Conversation
📝 WalkthroughWalkthroughThis pull request updates the library version from beta to release candidate by modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant U as Upgrade Handler
participant Disk as Disk (UpgradeInfo)
participant Keeper as UpgradeKeeper
participant Module as Move Module
U->>Keeper: ReadUpgradeInfoFromDisk()
Keeper-->>U: Return Upgrade Info / Error
U->>U: Evaluate upgrade conditions
alt Upgrade applicable
U->>Store: Create storeUpgrades (with dynamicfeetypes.StoreKey)
U->>Module: Run Migrations & Update Parameters
else
U->>U: Skip upgrade steps
end
sequenceDiagram
participant User as CLI User
participant CLI as Dynamic Fee CLI
participant Client as Query Client
participant Module as Dynamic Fee Module
User->>CLI: Execute query command
CLI->>Client: Build query client & send request
Client->>Module: Query dynamic-fee parameters
Module-->>Client: Return parameters
Client-->>CLI: Deliver response
CLI-->>User: Display parameters in protobuf format
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later OpenSSF Scorecard
Scanned Files
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/dynamic-fee/module.go (1)
139-143
: Revise the doc comment to match the actual return
The doc currently suggests "no root query command," but the function returns a valid query command. Consider adjusting the comment to be more precise.Apply this diff to correct the doc comment:
-// GetQueryCmd returns no root query command for the move module. +// GetQueryCmd integrates the dynamic-fee module's query commands into the move module.x/dynamic-fee/client/cli/query.go (2)
18-30
: Add integration tests for root query command
TheGetQueryCmd
function sets up a command tree for dynamic-fee queries. Consider adding integration tests covering both valid and invalid scenarios to ensure reliability.
32-66
: Test query params flow
TheGetCmdQueryParams
command logic looks correct for fetching and printing the module’s parameters. It’s recommended to add more tests or manual checks to confirm the output matches expectations under varied scenarios.app/upgrade.go (1)
19-22
: Revisit panic on failed read
Panicking on an upgrade info read error may prevent graceful handling or logging. Consider a more user-friendly approach to error handling if feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
📒 Files selected for processing (4)
Dockerfile
(1 hunks)app/upgrade.go
(1 hunks)x/dynamic-fee/client/cli/query.go
(1 hunks)x/dynamic-fee/module.go
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
x/dynamic-fee/module.go (1)
x/dynamic-fee/client/cli/query.go (1)
GetQueryCmd
(18-30)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (go)
- GitHub Check: Run test and upload codecov
- GitHub Check: golangci-lint
🔇 Additional comments (8)
Dockerfile (1)
11-11
:✅ Verification successful
Confirm reference to updated library version
This environment variable is updated to v1.0.0-rc.0. Please ensure that references to the library across the codebase are aligned and that no references remain pointing to the old version.You can run this script to search for library version references in the repository to confirm consistency:
🏁 Script executed:
#!/bin/bash rg "v1.0.0-beta.0|v1.0.0-rc.0" -A 2Length of output: 268
Library version update is consistent across the codebase.
The search confirmed that all instances of the library version have been updated to v1.0.0-rc.0, including inDockerfile
(line 11) andapp/upgrade.go
. No lingering references to the old version (v1.0.0-beta.0) were found.x/dynamic-fee/module.go (1)
9-17
: New imports for the CLI
The additions ofcobra
and the localcli
package are appropriate for providing a query command interface.x/dynamic-fee/client/cli/query.go (1)
1-17
: Imports for new dynamic-fee CLI
The chosen imports (Cobra, Cosmos SDK, etc.) align with implementing the new query capabilities.app/upgrade.go (5)
6-8
: Imports support upgrade mechanics
Usingcosmossdk.io/errors
,cosmossdk.io/math
, andstoretypes
provides enhanced error handling, math utilities, and store upgrade capabilities.
11-12
: Dynamic-fee store import
Importing the dynamic-fee module's types is necessary for referencing the store key in the upgrade steps.
15-15
: Upgrade name set to release candidate
The updated constantv1.0.0-rc.0
matches the Dockerfile’s version change, ensuring consistency.
24-33
: Conditional store upgrade loader
The logic to add the dynamic-fee store key if the upgrade is not skipped appears correct and aligns with the new release candidate.
38-55
: Validate zero gas price and fee parameter changes
SettingBaseMinGasPrice
and the dynamic feeMaxChangeRate
to zero effectively disables incremental fee increases. Confirm this is the desired behavior.Would you like a helper script to search for downstream references to these parameters to ensure no unintended side effects occur?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
==========================================
- Coverage 39.96% 39.86% -0.10%
==========================================
Files 292 293 +1
Lines 27141 27210 +69
==========================================
+ Hits 10846 10847 +1
- Misses 14621 14687 +66
- Partials 1674 1676 +2
🚀 New features to boost your workflow:
|
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
No description provided.