feat: set defaults for --v2-upgrade-height
#4091
Conversation
rootulp
requested review from
staheri14,
ninabarbakadze,
rach-id and
evan-forbes
and removed request for
a team and
staheri14
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
![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 (1)
cmd/celestia-appd/cmd/start.go (1)
121-133: Consider adding unit tests for chain ID handlingThe chain ID handling logic would benefit from unit tests to ensure correct behavior across different networks and edge cases.
Would you like me to help create unit tests for this functionality?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
cmd/celestia-appd/cmd/start.go(2 hunks)pkg/appconsts/chain_ids.go(1 hunks)pkg/appconsts/upgrade_heights.go(1 hunks)
🔇 Additional comments (3)
pkg/appconsts/chain_ids.go (1)
5-6: LGTM: Chain ID constants are well-defined
The new chain ID constants follow the existing naming convention and correctly define the identifiers for Mocha and Mainnet networks.
pkg/appconsts/upgrade_heights.go (1)
4-12: LGTM: Upgrade heights are well-documented
The upgrade height constants are clearly documented and properly defined for each network. However, let's verify these heights against any network documentation or announcements.
✅ Verification successful
Let me search for more specific references to these upgrade heights.
Upgrade heights are verified and consistently used across the codebase
The upgrade heights are verified through multiple sources in the codebase:
- The heights are consistently used in network startup scripts (
scripts/*.sh) - They are referenced in the blocketa tool as example heights
- The values are properly documented and used in the network upgrade process as evidenced by ADR-018 and release notes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any documentation validating these upgrade heights
# Look for upgrade height references in markdown files
rg -A 2 "upgrade.*height|v2.*height" --type md
Length of output: 10636
Script:
#!/bin/bash
# Search for specific upgrade heights in the entire codebase
rg -A 2 "1751707|2585031|2371495"
Length of output: 1681
cmd/celestia-appd/cmd/start.go (1)
17-17: LGTM: Required import added
The appconsts package import is correctly added to access the chain ID and upgrade height constants.
cmd/celestia-appd/cmd/start.go
Outdated
| switch clientCtx.ChainID { | ||
| case appconsts.ArabicaChainID: | ||
| serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.ArabicaUpgradeHeightV2, appconsts.ArabicaChainID)) | ||
| serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.ArabicaUpgradeHeightV2) | ||
| case appconsts.MochaChainID: | ||
| serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MochaUpgradeHeightV2, appconsts.MochaChainID)) | ||
| serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.MochaUpgradeHeightV2) | ||
| case appconsts.MainnetChainID: | ||
| serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MainnetChainID, appconsts.MainnetChainID)) | ||
| serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.MainnetUpgradeHeightV2) | ||
| default: | ||
| serverCtx.Logger.Info(fmt.Sprintf("No default value exists for the v2 upgrade height when the chainID is %v", clientCtx.ChainID)) | ||
| } |
There was a problem hiding this comment.
Fix incorrect log messages and improve code structure
There are several issues in the chain ID handling logic:
- In the MochaChainID case, the log message swaps the upgrade height and chain ID values
- In the MainnetChainID case, the log message uses MainnetChainID twice instead of MainnetUpgradeHeightV2
- Consider extracting the upgrade height flag string to a constant
Apply these fixes:
switch clientCtx.ChainID {
case appconsts.ArabicaChainID:
serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.ArabicaChainID, appconsts.ArabicaUpgradeHeightV2))
serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.ArabicaUpgradeHeightV2)
case appconsts.MochaChainID:
- serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MochaUpgradeHeightV2, appconsts.MochaChainID))
+ serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MochaChainID, appconsts.MochaUpgradeHeightV2))
serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.MochaUpgradeHeightV2)
case appconsts.MainnetChainID:
- serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MainnetChainID, appconsts.MainnetChainID))
+ serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.MainnetChainID, appconsts.MainnetUpgradeHeightV2))
serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.MainnetUpgradeHeightV2)Consider extracting the upgrade height flag to a constant:
+const upgradeHeightFlagName = "v2-upgrade-height"
+
switch clientCtx.ChainID {
case appconsts.ArabicaChainID:
serverCtx.Logger.Info(fmt.Sprintf("Since the chainID is %v, configuring the default v2 upgrade height to %v", appconsts.ArabicaChainID, appconsts.ArabicaUpgradeHeightV2))
- serverCtx.Viper.SetDefault(UpgradeHeightFlag, appconsts.ArabicaUpgradeHeightV2)
+ serverCtx.Viper.SetDefault(upgradeHeightFlagName, appconsts.ArabicaUpgradeHeightV2)Committable suggestion skipped: line range outside the PR's diff.
rootulp
changed the title
--v2-upgrade-height
Closes celestiaorg#3995 If a user specifies the `--v2-upgrade-height` flag then it takes precedence over any defaults. If the user doesn't specify a `--v2-upgrade-height` flag and the chainID is `arabica-11`, `mocha-4`, or `celestia` then the v2 upgrade height is set to the block height that was coordinated on those chains. See: https://docs.celestia.org/how-to-guides/network-upgrade-process#lemongrass-network-upgrade ## Testing 1. `./scripts/single-node.sh` doesn't set a v2 upgrade height 2. `./arabica.sh` if we remove the `--v2-upgrade-height` flag in the script, gets defaulted correctly 3. `./arabica.sh` if we override the `--v2-upgrade-height` flag in the script, that gets used
Closes celestiaorg#3995 If a user specifies the `--v2-upgrade-height` flag then it takes precedence over any defaults. If the user doesn't specify a `--v2-upgrade-height` flag and the chainID is `arabica-11`, `mocha-4`, or `celestia` then the v2 upgrade height is set to the block height that was coordinated on those chains. See: https://docs.celestia.org/how-to-guides/network-upgrade-process#lemongrass-network-upgrade ## Testing 1. `./scripts/single-node.sh` doesn't set a v2 upgrade height 2. `./arabica.sh` if we remove the `--v2-upgrade-height` flag in the script, gets defaulted correctly 3. `./arabica.sh` if we override the `--v2-upgrade-height` flag in the script, that gets used
Closes #3995
If a user specifies the
--v2-upgrade-heightflag then it takes precedence over any defaults. If the user doesn't specify a--v2-upgrade-heightflag and the chainID isarabica-11,mocha-4, orcelestiathen the v2 upgrade height is set to the block height that was coordinated on those chains.See: https://docs.celestia.org/how-to-guides/network-upgrade-process#lemongrass-network-upgrade
Testing
./scripts/single-node.shdoesn't set a v2 upgrade height./arabica.shif we remove the--v2-upgrade-heightflag in the script, gets defaulted correctly./arabica.shif we override the--v2-upgrade-heightflag in the script, that gets used