[OTE-829] upgrade handler for affiliates #2332
Conversation
|
Warning Rate limit exceeded@teddyding has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce modifications to the upgrade handling process within the protocol. The Changes
Possibly related PRs
Suggested labels
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
protocol/app/upgrades/v7.0.0/upgrade.go (2)
60-62: LGTM: New function to set default affiliate tiers.The
setDefaultTiersForAffiliatesfunction is well-implemented and serves its purpose of updating affiliate tiers with default values during the upgrade process.Consider adding a log message to indicate that the default tiers have been set. This can be helpful for debugging and monitoring the upgrade process. For example:
func setDefaultTiersForAffiliates(ctx sdk.Context, k affiliateskeeper.Keeper) { k.UpdateAffiliateTiers(ctx, affiliatetypes.DefaultAffiliateTiers) + ctx.Logger().Info("Default affiliate tiers have been set") }
69-69: LGTM: Updated CreateUpgradeHandler with affiliate tier initialization.The changes to
CreateUpgradeHandlerare well-implemented:
- The function signature is correctly updated to include
affiliatesKeeper.- The new
setDefaultTiersForAffiliatesfunction is called at an appropriate point in the upgrade process.Consider adding error handling for the
setDefaultTiersForAffiliatescall. While the current implementation assumes this operation always succeeds, it's generally a good practice to handle potential errors, especially during critical processes like upgrades. For example:// Set default tiers for affiliates. -setDefaultTiersForAffiliates(sdkCtx, affiliatesKeeper) +if err := setDefaultTiersForAffiliates(sdkCtx, affiliatesKeeper); err != nil { + return nil, fmt.Errorf("failed to set default affiliate tiers: %w", err) +}This would require updating the
setDefaultTiersForAffiliatesfunction to return an error if theUpdateAffiliateTierscall fails.Also applies to: 81-83
protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2)
104-119: LGTM: New function to check affiliate tiers post-upgrade.The
postUpgradeAffiliatesModuleTiersCheckfunction is well-implemented and correctly verifies that the affiliates module has been initialized with the default tiers after the upgrade. The query, unmarshalling, and assertion are all handled appropriately.Consider adding an assertion to check the length of
affiliateTiersResp.Tiersbefore comparing it withaffiliatestypes.DefaultAffiliateTiers. This would provide a more informative error message if the lengths don't match. For example:require.NoError(t, err) +require.Equal(t, len(affiliatestypes.DefaultAffiliateTiers), len(affiliateTiersResp.Tiers), "Number of tiers doesn't match") require.Equal(t, affiliatestypes.DefaultAffiliateTiers, affiliateTiersResp.Tiers)
Line range hint
1-119: Overall assessment: Well-implemented upgrade test for affiliates module.The changes in this file effectively implement the necessary tests for the upgrade handler, with a focus on verifying the correct initialization of the affiliates module. The new
postUpgradeAffiliatesModuleTiersCheckfunction is well-integrated into the existing test structure and provides good coverage for the upgrade process.Consider adding similar post-upgrade checks for other modules that might be affected by the upgrade, if any. This would ensure comprehensive testing of all upgrade-related changes across the system.
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 not posted (4)
protocol/app/upgrades.go (1)
35-35: LGTM: Addition of AffiliatesKeeper to upgrade handler.The addition of
app.AffiliatesKeeperas an argument toCreateUpgradeHandleris appropriate if there are new upgrade requirements related to affiliates. This change allows the upgrade handler to access and potentially modify affiliate data during the upgrade process.To ensure consistency, let's verify the
CreateUpgradeHandlerfunction definition and its usage:Please review the output to confirm that the
CreateUpgradeHandlerfunction definition includesAffiliatesKeeperas a parameter and that all other usages of this function are updated accordingly.Verification successful
: The addition of
app.AffiliatesKeeperto theCreateUpgradeHandlerfunction is correctly implemented. The function definition includesaffiliatesKeeper affiliateskeeper.Keeper, and all relevant usages have been updated accordingly. No issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the CreateUpgradeHandler function definition and its usage # Check the CreateUpgradeHandler function definition echo "Checking CreateUpgradeHandler function definition:" rg --type go "func\s+CreateUpgradeHandler" -A 10 # Check other usages of CreateUpgradeHandler echo "\nChecking other usages of CreateUpgradeHandler:" rg --type go "CreateUpgradeHandler\(" -A 5Length of output: 10138
protocol/app/upgrades/v7.0.0/upgrade.go (1)
12-13: LGTM: New imports for affiliates functionality.The new imports for
affiliateskeeperandaffiliatetypesare correctly added and follow the existing import style of the file. These imports are necessary for the new affiliate-related functionality introduced in this upgrade.protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2)
14-14: LGTM: New import for affiliates types.The new import for
affiliatestypesis correctly added and necessary for the new affiliate-related functionality.
52-54: LGTM: New affiliate module check added to post-upgrade checks.The new function call
postUpgradeAffiliatesModuleTiersCheckis appropriately added to thepostUpgradeChecksfunction. The comment accurately describes its purpose, ensuring that the affiliates module has been initialized with the default tiers after the upgrade.
| setDefaultTiersForAffiliates(sdkCtx, affiliatesKeeper) | ||
|
|
||
| // Set the module version to 1 to skip init genesis during the first upgrade. | ||
| vm[affiliatetypes.ModuleName] = 1 |
There was a problem hiding this comment.
Why is this needed for x/affiliate which is a new module that didn't exist in v6?
There was a problem hiding this comment.
its because RunMigrations calls InitGenesis here
There was a problem hiding this comment.
which overwrites setDefaultTiersForAffiliates
|
https://github.com/Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 6f9ede5) # Conflicts: # protocol/app/upgrades/v7.0.0/upgrade_container_test.go
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
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