[TRA-654] add listing module state init into v7 upgrade handler #2432
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Add listing module store to the upgrades here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
protocol/app/upgrades/v7.0.0/upgrade_container_test.go (1)
212-241: LGTM: New function for checking listing module state.The
postUpgradeListingModuleStateCheckfunction is well-implemented and correctly verifies the initialization of the listing module state. It follows the existing patterns in the file and uses appropriate error handling.Consider extracting the magic number
500used for the markets hard cap into a named constant at the top of the file for better maintainability and clarity.protocol/app/upgrades/v7.0.0/upgrade.go (1)
200-215: Consider defining the hard cap value as a constantThe hard-coded value
500inSetMarketsHardCapcould be defined as a constant to enhance readability and maintainability. This practice makes it easier to manage and update the value in the future.Apply this diff to define the hard cap as a constant:
+const ( + MarketsHardCap = 500 +) + func initListingModuleState(ctx sdk.Context, listingKeeper listingkeeper.Keeper) { // Set hard cap on listed markets - err := listingKeeper.SetMarketsHardCap(ctx, 500) + err := listingKeeper.SetMarketsHardCap(ctx, MarketsHardCap) if err != nil { panic(fmt.Sprintf("failed to set markets hard cap: %s", err)) } // Set listing vault deposit params err = listingKeeper.SetListingVaultDepositParams( ctx, listingtypes.DefaultParams(), ) if err != nil { panic(fmt.Sprintf("failed to set listing vault deposit params: %s", err)) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v7.0.0/upgrade.go (3 hunks)
- protocol/app/upgrades/v7.0.0/upgrade_container_test.go (3 hunks)
🔇 Additional comments (6)
protocol/app/upgrades.go (1)
36-36: 💡 Codebase verificationListingKeeper Not Utilized in v7.0.0 Upgrade Handler
The
ListingKeeperis added as a parameter to thev7_0_0.CreateUpgradeHandlercall inprotocol/app/upgrades.go. However, the current implementation of thev7.0.0upgrade handler does not utilize theListingKeeper. This may result in the listing module state not being properly initialized during the upgrade.Next Steps:
- Ensure that
v7_0_0.CreateUpgradeHandlercorrectly integrates and utilizes theListingKeeperto handle listing module state initialization.🔗 Analysis chain
LGTM. Verify v7.0.0 upgrade handler implementation.
The addition of
app.ListingKeeperto thev7_0_0.CreateUpgradeHandlercall is consistent with the PR objective of integrating the listing module state initialization into the v7 upgrade handler. This change looks good and aligns with the expected functionality.To ensure full compatibility, please verify that the v7.0.0 upgrade handler implementation correctly utilizes the
ListingKeeper. Run the following script to check the implementation:This script will help confirm that the
ListingKeeperis properly integrated into the v7.0.0 upgrade logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the v7.0.0 upgrade handler uses ListingKeeper # Test: Search for ListingKeeper usage in the v7.0.0 upgrade handler rg --type go -A 5 'ListingKeeper' $(fd -t f -e go v7_0_0)Length of output: 7460
protocol/app/upgrades/v7.0.0/upgrade_container_test.go (3)
9-10: LGTM: Import statement for listing module types.The addition of the
listingtypesimport is consistent with the PR objectives and follows Go conventions.
63-65: LGTM: Addition of listing module state check.The new function call
postUpgradeListingModuleStateCheckis appropriately placed within thepostUpgradeChecksfunction and aligns with the PR objectives.
Line range hint
1-241: Summary: Changes align well with PR objectives.The modifications to this file successfully integrate the listing module state initialization into the v7 upgrade handler test suite. The changes are consistent, well-structured, and follow the established patterns in the file. They effectively address the PR objectives by:
- Adding the necessary import for the listing module types.
- Incorporating a new check in the
postUpgradeChecksfunction.- Implementing a detailed
postUpgradeListingModuleStateCheckfunction to verify the correct initialization of the listing module state.These changes enhance the test coverage for the upgrade process, ensuring that the listing module is properly initialized during the upgrade.
protocol/app/upgrades/v7.0.0/upgrade.go (2)
241-243: Listing module state initialization is correctly integratedThe call to
initListingModuleStatewithin theCreateUpgradeHandlerensures that the listing module state is properly initialized during the upgrade. This integration aligns with the upgrade process and sets up the necessary state for the listing module.
223-223: Ensure all invocations ofCreateUpgradeHandlerare updated with the new parameterThe function
CreateUpgradeHandlernow includes an additional parameterlistingKeeper listingkeeper.Keeper. Please verify that all calls to this function throughout the codebase have been updated accordingly to prevent build failures or unexpected behavior.Run the following script to find all usages of
CreateUpgradeHandlerand check their parameters:✅ Verification successful
All invocations of
CreateUpgradeHandlerhave been verified and no usages were found in the codebase. No further action is needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to CreateUpgradeHandler include the new 'listingKeeper' parameter. # Test: Search for function calls to CreateUpgradeHandler. # Expect: All calls should have the updated parameter list. ast-grep --lang go --pattern $'CreateUpgradeHandler($$$)' \ --json --pretty \ | jq -r '.matches[].match | .text' \ | while read -r line; do echo "Found usage: $line" doneLength of output: 385
Script:
#!/bin/bash # Description: Verify that all calls to CreateUpgradeHandler include the new 'listingKeeper' parameter without using unsupported flags. # Test: Search for function calls to CreateUpgradeHandler. # Expect: All calls should have the updated parameter list. ast-grep --lang go --pattern $'CreateUpgradeHandler($$$)' \ --json \ | jq -r '.matches[].match | .text' \ | while read -r line; do echo "Found usage: $line" doneLength of output: 182
Script:
#!/bin/bash # Description: Inspect raw JSON output from CreateUpgradeHandler searches to determine correct jq parsing. # Test: Output raw JSON from ast-grep without parsing. # Expect: To understand the structure of the JSON for accurate jq filtering. ast-grep --lang go --pattern $'CreateUpgradeHandler($$$)' \ --json \ > astgrep_output.json cat astgrep_output.jsonLength of output: 94
| func initListingModuleState(ctx sdk.Context, listingKeeper listingkeeper.Keeper) { | ||
| // Set hard cap on listed markets | ||
| err := listingKeeper.SetMarketsHardCap(ctx, 500) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to set markets hard cap: %s", err)) | ||
| } | ||
|
|
||
| // Set listing vault deposit params | ||
| err = listingKeeper.SetListingVaultDepositParams( | ||
| ctx, | ||
| listingtypes.DefaultParams(), | ||
| ) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to set listing vault deposit params: %s", err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review error handling for better upgrade resilience
While panicking on errors during state initialization ensures that issues are caught immediately, it may be beneficial to handle errors more gracefully to prevent the entire upgrade process from failing unexpectedly. Consider logging the errors and returning them up the call stack if appropriate.
Adjust the error handling to return errors:
func initListingModuleState(ctx sdk.Context, listingKeeper listingkeeper.Keeper) error {
// Set hard cap on listed markets
err := listingKeeper.SetMarketsHardCap(ctx, MarketsHardCap)
if err != nil {
- panic(fmt.Sprintf("failed to set markets hard cap: %s", err))
+ return fmt.Errorf("failed to set markets hard cap: %w", err)
}
// Set listing vault deposit params
err = listingKeeper.SetListingVaultDepositParams(
ctx,
listingtypes.DefaultParams(),
)
if err != nil {
- panic(fmt.Sprintf("failed to set listing vault deposit params: %s", err))
+ return fmt.Errorf("failed to set listing vault deposit params: %w", err)
}
+ return nil
}Update the call in CreateUpgradeHandler:
// Initialize listing module state.
- initListingModuleState(sdkCtx, listingKeeper)
+ if err := initListingModuleState(sdkCtx, listingKeeper); err != nil {
+ return nil, err
+ }Committable suggestion was skipped due to low confidence.
| @@ -194,12 +197,30 @@ func migrateVaultSharesToMegavaultShares(ctx sdk.Context, k vaultkeeper.Keeper) | |||
| ctx.Logger().Info("Successfully migrated vault shares to megavault shares") | |||
| } | |||
|
|
|||
| func initListingModuleState(ctx sdk.Context, listingKeeper listingkeeper.Keeper) { | |||
| // Set hard cap on listed markets | |||
| err := listingKeeper.SetMarketsHardCap(ctx, 500) | |||
There was a problem hiding this comment.
nit: Do we want to add this as a constant in the listing module rather than a magic number here?
There was a problem hiding this comment.
A default number in the listing module can change down the line so don't want to reference it here. This code is also just gonna run once
There was a problem hiding this comment.
Add to constants.go instead then?
The listing module store key was already added in v6. Do I need it again here? |
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- protocol/app/upgrades/v7.0.0/upgrade.go (3 hunks)
- protocol/x/listing/types/constants.go (1 hunks)
🔇 Additional comments (5)
protocol/app/upgrades/v7.0.0/upgrade.go (5)
8-9: Import statement forlistingtypesadded correctlyThe import for
listingtypesis appropriately added and necessary for the listing module functionality.
17-17: Import statement forlistingkeeperadded correctlyThe import for
listingkeeperis appropriately added to access the listing module's keeper.
200-215: Initialization of listing module state is implemented correctlyThe
initListingModuleStatefunction properly initializes the listing module state by setting the markets hard cap and listing vault deposit parameters using the default values.
223-223: AddedlistingKeeperparameter toCreateUpgradeHandlerThe additional
listingKeeperparameter inCreateUpgradeHandleris correctly incorporated to pass the listing keeper to the upgrade handler.
241-243: Listing module state initialized within the upgrade handlerThe call to
initListingModuleStateensures that the listing module state is initialized during the upgrade process, which is necessary for the module's proper operation post-upgrade.
|
|
||
| DefaultMarketsHardCap = 500 |
There was a problem hiding this comment.
💡 Codebase verification
Add a descriptive comment for DefaultMarketsHardCap
The constant DefaultMarketsHardCap is correctly added and utilized in the upgrade process. To enhance code clarity and maintainability, please add a comment explaining its purpose and the rationale behind the chosen value. For example:
// DefaultMarketsHardCap defines the maximum number of markets that can be listed.
// The value of 500 is based on [insert rationale here].
DefaultMarketsHardCap = 500🔗 Analysis chain
New constant DefaultMarketsHardCap added
The addition of DefaultMarketsHardCap with a value of 500 seems appropriate. However, consider the following points:
- The constant name follows the existing naming convention (PascalCase), which is good.
- The value 500 appears to be a reasonable default for a markets hard cap, but it would be helpful to add a comment explaining the rationale behind this specific value.
- Consider adding a unit to the constant name for clarity, e.g.,
DefaultMarketsHardCapCount.
Consider adding a brief comment above the constant to explain its purpose and the reasoning behind the chosen value. For example:
// DefaultMarketsHardCap defines the maximum number of markets that can be listed.
// This value was chosen based on [insert rationale here].
DefaultMarketsHardCap = 500To ensure this constant is used appropriately throughout the codebase, let's search for its usage:
To ensure DefaultMarketsHardCap is utilized correctly, let's examine the implementation of the SetMarketsHardCap method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of DefaultMarketsHardCap
rg --type go "DefaultMarketsHardCap"
Length of output: 224
Script:
#!/bin/bash
# Find the implementation of SetMarketsHardCap method
rg --type go "func.*SetMarketsHardCap"
Length of output: 4100
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
Signed-off-by: Shrenuj Bansal <[email protected]> (cherry picked from commit 922a9b5)
…port #2432) (#2445) Co-authored-by: shrenujb <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Changelist
Set markets hard cap and default listing vault params for the listing module state in v7 upgrade handler
Test Plan
Existing tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests