Drop patch number from upgrade name, v7.0.0 to v7.0 #2512
Conversation
adamfraser
force-pushed
the
adam/change-upgrade-name-v7.0
branch
from
3908f16 to
de0d889
Compare
WalkthroughThe pull request modifies versioning constants and package names across several files. 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 (
|
adamfraser
force-pushed
the
adam/change-upgrade-name-v7.0
branch
from
de0d889 to
452c16d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
protocol/app/upgrades/v7.0/upgrade.go (5)
Line range hint
29-31: Update panic message to accurately reflect behavior.In the
initializeModuleAccsfunction, the panic message states "This is not expected. Skipping.", but since the code panics, execution will halt immediately rather than skip processing. Consider updating the message to accurately reflect that the upgrade cannot proceed.Apply this diff to update the message:
- panic(fmt.Sprintf( - "Did not find %v in `ak.GetModuleAddressAndPermissions`. This is not expected. Skipping.", + panic(fmt.Sprintf( + "Did not find %v in `ak.GetModuleAddressAndPermissions`. Cannot proceed with upgrade.", modAccName, ))
Line range hint
119-122: Handle potential division by zero in owner shares calculation.In the
migrateVaultSharesToMegavaultSharesfunction, when calculatingownerShares, ensure thatequity.Denom()is not zero to prevent a division by zero panic.Consider adding a check before performing the division:
if equity.Denom().Sign() == 0 { ctx.Logger().Warn(fmt.Sprintf( "Owner %s has zero equity denominator, skipping calculation", owner, )) continue }
Line range hint
72-76: Avoid panicking on setting vault params failure.In the
migrateVaultQuotingParamsToVaultParamsfunction, panicking whenSetVaultParamsfails may halt the entire upgrade process. Consider handling the error to allow the upgrade to continue or to provide better diagnostic information.Update the error handling as follows:
if err != nil { ctx.Logger().Error(fmt.Sprintf( "Failed to set vault params for vault %v with params %v: %s", vaultId, vaultParams, err, )) // Decide whether to continue or rollback based on the severity. // For critical errors, you might want to return or panic. continue }
Line range hint
203-205: Enhance logging after module migrations.After running
mm.RunMigrations, it would be helpful to log the successful migration of each module to ensure all migrations have been applied as expected.Consider adding detailed logging:
newVM, err := mm.RunMigrations(ctx, configurator, vm) if err != nil { return nil, err } for moduleName, moduleVersion := range newVM { sdkCtx.Logger().Info(fmt.Sprintf("Module %s migrated to version %d", moduleName, moduleVersion)) } return newVM, nil
Line range hint
8-27: Consider reusing module account initialization logic.The
initializeModuleAccsfunction mentions that the logic is copied from the v3.0.0 upgrade handler. To promote code reuse and maintainability, consider abstracting this logic into a shared utility function or method if it's used across multiple versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- protocol/app/upgrades.go (2 hunks)
- protocol/app/upgrades/v7.0/constants.go (2 hunks)
- protocol/app/upgrades/v7.0/upgrade.go (1 hunks)
- protocol/app/upgrades/v7.0/upgrade_container_test.go (3 hunks)
- protocol/testing/version/VERSION_CURRENT (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/app/upgrades/v7.0/constants.go
- protocol/app/upgrades/v7.0/upgrade_container_test.go
- protocol/testing/version/VERSION_CURRENT
🧰 Additional context used
🔇 Additional comments (6)
protocol/app/upgrades.go (5)
17-17: LGTM. Upgrade reference updated correctly.The Upgrades slice has been updated to use the new v7_0.Upgrade reference, which is consistent with the import statement change and the PR objectives.
25-26: LGTM. Upgrade name references updated correctly.The HasHandler check and the panic message have been updated to use the new v7_0.UpgradeName reference, which is consistent with the previous changes and the PR objectives.
6-6: LGTM. Verify the new import path.The import statement has been correctly updated to reflect the new version without the patch number, which is consistent with the PR objectives.
Let's verify that the new import path exists:
#!/bin/bash # Verify that the new import path exists fd -t d "v7\.0$" protocol/app/upgrades
29-30: LGTM. Upgrade handler references updated correctly.The SetUpgradeHandler call has been updated to use the new v7_0.UpgradeName and v7_0.CreateUpgradeHandler references, which is consistent with the previous changes and the PR objectives.
To ensure the upgrade handler's functionality remains intact, let's verify its implementation:
Line range hint
6-30: Summary: Version update successfully implementedThe changes in this file consistently update the upgrade name from v7.0.0 to v7.0 across import statements, upgrade references, and handler registrations. These modifications align with the PR objectives to drop the patch number from the upgrade name.
Key points:
- Import path updated
- Upgrades slice reference updated
- HasHandler check and panic message updated
- SetUpgradeHandler call updated
The changes appear to be complete and correct for this file. However, ensure that any other files referencing the old version (v7.0.0) are also updated accordingly throughout the codebase.
Let's verify if there are any remaining references to the old version:
protocol/app/upgrades/v7.0/upgrade.go (1)
1-1: Verify package name change impacts.The package name has been changed from
v_7_0_0tov_7_0. Ensure that all references to this package throughout the codebase have been updated to prevent import errors or inconsistencies.Run the following script to find any remaining references to the old package name:
adamfraser
force-pushed
the
adam/change-upgrade-name-v7.0
branch
from
452c16d to
296deda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/app/upgrades/v7.0/upgrade.go (2)
Line range hint
23-56: Refactor to eliminate code duplicationThe
initializeModuleAccsfunction duplicates module account initialization logic from thev3.0.0upgrade handler. To enhance maintainability and reduce code duplication, consider abstracting this logic into a shared utility function that can be reused across multiple upgrade handlers.
Line range hint
107-109: Prevent potential division by zero inownerSharescalculationIn the calculation of
ownerShares, there is a potential risk of division by zero ifequity.Denom()is zero. To avoid a runtime panic, add a check to ensure thatequity.Denom()is not zero before performing the division.Apply this diff to add the zero denominator check:
+ if equity.Denom().Sign() == 0 { + ctx.Logger().Warn(fmt.Sprintf( + "Owner %s has zero denominator in equity: %s", + owner, + equity, + )) + continue + } ownerShares := new(big.Int).Quo( equity.Num(), equity.Denom(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- protocol/app/upgrades.go (2 hunks)
- protocol/app/upgrades/v7.0/constants.go (2 hunks)
- protocol/app/upgrades/v7.0/upgrade.go (1 hunks)
- protocol/app/upgrades/v7.0/upgrade_container_test.go (3 hunks)
- protocol/testing/version/VERSION_CURRENT (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- protocol/app/upgrades.go
- protocol/app/upgrades/v7.0/constants.go
- protocol/app/upgrades/v7.0/upgrade_container_test.go
- protocol/testing/version/VERSION_CURRENT
🧰 Additional context used
🔇 Additional comments (1)
protocol/app/upgrades/v7.0/upgrade.go (1)
1-1: Ensure package name consistency across the codebaseThe package name has been updated to
v_7_0. Please verify that all references and import statements in the codebase have been updated accordingly to prevent any broken imports.
|
@mergify backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 66d7a5d)
…2515) Co-authored-by: Adam Fraser <[email protected]>
Changelist
Modifies the upgrade name to follow the new convention of not including minor patch numbers.
v7.0.0 to v7.0
Summary by CodeRabbit
New Features
v7.0.0tov7.0).Bug Fixes
Documentation