feat: v8.0.0-rc.4 upgrade handler
#422
Conversation
WalkthroughThe changes in this pull request involve modifications primarily to the Changes
Possibly related PRs
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 (
|
johnletey
changed the title
v8.0.0-rc.4 upgrade handler
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (16)
e2e/module_check_test.go (1)
Line range hint
31-37: Consider enhancing test documentation and coverage.While the test logic is sound, consider these improvements:
- Add a comment explaining why these specific modules are restricted
- Make error messages more descriptive (e.g., include expected behavior)
- Consider adding test cases for non-restricted modules to ensure complete coverage
Example enhancement:
func TestRestrictedModules(t *testing.T) { t.Parallel() + // These modules are restricted for security reasons: + // - circuit: prevents unauthorized state transitions + // - gov: prevents unauthorized governance actions + // - group: prevents unauthorized group management ctx := context.Background() nw, _ := e2e.NobleSpinUp(t, ctx, e2e.LocalImages, false) noble := nw.Chain.GetNode() restrictedModules := []string{"circuit", "gov", "group"} for _, module := range restrictedModules { - require.False(t, noble.HasCommand(ctx, "query", module), fmt.Sprintf("%s is a restricted module", module)) + require.False(t, noble.HasCommand(ctx, "query", module), + fmt.Sprintf("module %s should be restricted and not have query commands available", module)) } + + // Verify non-restricted modules are accessible + allowedModules := []string{"bank", "staking"} + for _, module := range allowedModules { + require.True(t, noble.HasCommand(ctx, "query", module), + fmt.Sprintf("module %s should be accessible and have query commands available", module)) + } }upgrade/testnet/store.go (1)
24-30: Implementation looks good, but consider adding validation.The store loader implementation correctly adds the globalfee module. However, consider adding validation for the upgradeHeight parameter to ensure it's positive.
func CreateStoreLoader(upgradeHeight int64) baseapp.StoreLoader { + if upgradeHeight <= 0 { + panic("upgrade height must be positive") + } storeUpgrades := storetypes.StoreUpgrades{ Added: []string{globalfeetypes.ModuleName}, }e2e/upgrade_test.go (1)
29-29: Consider making the genesis version configurable.The hardcoded genesis version might make the test brittle and harder to maintain. Consider making it configurable through a constant or environment variable, especially since this is a testnet upgrade handler.
+const defaultGenesisVersion = "v8.0.0-rc.3-fix" + func TestChainUpgrade(t *testing.T) { - genesisVersion := "v8.0.0-rc.3-fix" + genesisVersion := os.Getenv("GENESIS_VERSION") + if genesisVersion == "" { + genesisVersion = defaultGenesisVersion + }e2e/cctp_receive_message_test.go (1)
45-45: Document the NobleSpinUp parameters and return values.The function call has been updated to include
e2e.LocalImagesand now returns a second value that's being discarded. Please add comments explaining:
- The purpose of the
e2e.LocalImagesparameter- What the discarded return value represents
e2e/cctp_receive_message_with_caller_test.go (2)
46-46: Consider handling the ignored return value from NobleSpinUp.The second return value from
NobleSpinUpis being discarded. If this value contains important information (like cleanup functions or error details), it should be properly handled.
Line range hint
1-249: LGTM! Well-structured E2E test implementation.The test provides comprehensive coverage of the CCTP message flow with proper setup, execution, and verification. It includes:
- Proper attestation setup and signature handling
- Token pair linking
- Minter controller configuration
- Message construction and verification
- Balance verification
A few suggestions to enhance the test further:
- Consider adding negative test cases (e.g., invalid signatures, wrong message format)
- Add cleanup steps to reset the chain state after the test
- Consider parameterizing test values (amounts, addresses) for better test maintainability
e2e/cctp_deposit_for_burn_test.go (1)
41-41: LGTM! Consider adding timeout configuration.The addition of
e2e.LocalImagesparameter aligns with the PR's objective of modifying the test environment setup.Consider adding a timeout configuration for
NobleSpinUpto ensure test stability:- nw, _ := e2e.NobleSpinUp(t, ctx, e2e.LocalImages, true) + spinupCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + nw, _ := e2e.NobleSpinUp(t, spinupCtx, e2e.LocalImages, true)e2e/cctp_deposit_for_burn_with_caller_test.go (1)
Line range hint
27-190: Consider enhancing test coverage with additional scenarios.While the test thoroughly covers the happy path, consider adding:
- Negative test cases (e.g., insufficient balance, invalid addresses)
- Edge cases (e.g., zero amounts, maximum amounts)
- Table-driven tests for different scenarios
This would provide more comprehensive validation of the CCTP functionality.
e2e/cctp_replace_deposit_for_burn_test.go (1)
Line range hint
1-259: Well-structured and comprehensive test implementation.The test thoroughly covers the CCTP replace deposit for burn functionality with:
- Proper setup of attesters and token pairs
- Comprehensive event verification
- Strong error handling
- Clear test flow and assertions
However, consider adding comments to explain the significance of magic numbers and constants:
- Domain IDs (e.g.,
SourceDomain: 4)- Byte lengths (e.g.,
make([]byte, 32))- Test values (e.g.,
"12345678901234567890123456789012")e2e/upgrade_utils.go (3)
57-57: Reconsider the use oft.Parallel()due to potential resource conflictsCalling
t.Parallel()allows the test to run concurrently with other tests. Since this test manipulates shared resources such as Docker containers and network configurations, running it in parallel may lead to resource conflicts, race conditions, or flakiness in tests.Consider removing
t.Parallel()to ensure that the test runs sequentially and does not interfere with other tests:if testing.Short() { t.Skip() } - t.Parallel()Alternatively, ensure that the test environment is isolated and that concurrent execution will not cause issues.
91-93: Avoid deferring context cancellation inside loops to prevent resource leaksAlthough this instance is not inside a loop, it's good practice to be consistent with context cancellation. Since the context
timeoutCtxis only used within a short scope, consider cancelling it immediately after use to release resources promptly.Modify the code to call
timeoutCtxCancel()immediately after it's no longer needed:timeoutCtx, timeoutCtxCancel := context.WithTimeout(ctx, time.Second*45) - defer timeoutCtxCancel() + err = testutil.WaitForBlocks(timeoutCtx, int(blocksAfterUpgrade), noble) require.NoError(t, err, "chain did not produce blocks after emergency upgrade") + timeoutCtxCancel()
91-96: Refactor duplicated code to improve maintainabilityThe code for creating a timeout context, waiting for blocks, and handling errors is duplicated in multiple places. This repetition can make the code harder to maintain and more prone to errors.
Consider extracting this logic into a helper function, such as
WaitForBlocksWithTimeout, to reduce duplication and improve readability. Here's an example:func WaitForBlocksWithTimeout(ctx context.Context, duration time.Duration, blocks int, chain *cosmos.CosmosChain) error { timeoutCtx, timeoutCtxCancel := context.WithTimeout(ctx, duration) defer timeoutCtxCancel() return testutil.WaitForBlocks(timeoutCtx, blocks, chain) }Then, replace the duplicated code with calls to this helper function:
- timeoutCtx, timeoutCtxCancel := context.WithTimeout(ctx, time.Second*45) - defer timeoutCtxCancel() - err = testutil.WaitForBlocks(timeoutCtx, int(blocksAfterUpgrade), noble) + err = WaitForBlocksWithTimeout(ctx, time.Second*45, int(blocksAfterUpgrade), noble) require.NoError(t, err, "chain did not produce blocks after upgrade")This refactoring improves code clarity and simplifies future maintenance.
Also applies to: 116-120, 123-128, 136-141, 210-215
e2e/utils.go (4)
Line range hint
118-122: Rename parametersnvandnffor clarityThe parameters
nvandnfrepresent the number of validators and full nodes, respectively. Renaming them tonumValidatorsandnumFullNodeswould improve code readability and maintainability.
Line range hint
118-122: Consider renamingversionparameter toimagesThe parameter
versionis an array ofibc.DockerImage. Renaming it toimagesordockerImageswould make the code more intuitive and better reflect its purpose.
393-395: Update parameter names inNobleSpinUpfunctionIn line with the previous suggestion, consider renaming the
versionparameter toimagesfor consistency and clarity.
437-440: Consistency in parameter naming forNobleSpinUpIBCSimilarly, update the
versionparameter toimagesin theNobleSpinUpIBCfunction to maintain consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
e2e/go.modis excluded by!**/*.mode2e/go.sumis excluded by!**/*.sum,!**/*.sumgo.work.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
Makefile(1 hunks)app.go(4 hunks)cmd/init.go(1 hunks)cmd/nobled/main.go(1 hunks)e2e/cctp_deposit_for_burn_test.go(1 hunks)e2e/cctp_deposit_for_burn_with_caller_test.go(1 hunks)e2e/cctp_receive_message_test.go(1 hunks)e2e/cctp_receive_message_with_caller_test.go(1 hunks)e2e/cctp_replace_deposit_for_burn_test.go(1 hunks)e2e/cctp_roles_test.go(4 hunks)e2e/conformance_test.go(1 hunks)e2e/fiat_tf_test.go(21 hunks)e2e/module_check_test.go(1 hunks)e2e/upgrade_test.go(1 hunks)e2e/upgrade_utils.go(1 hunks)e2e/utils.go(8 hunks)upgrade/testnet/constants.go(1 hunks)upgrade/testnet/store.go(1 hunks)upgrade/testnet/upgrade.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- cmd/init.go
- cmd/nobled/main.go
- e2e/fiat_tf_test.go
- upgrade/testnet/constants.go
🧰 Additional context used
🪛 Gitleaks
e2e/upgrade_utils.go
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (19)
e2e/conformance_test.go (1)
31-31: LGTM! Verify consistent usage across test files.
The addition of e2e.LocalImages parameter aligns with the PR's objective of modifying the upgrade handler and test environment setup.
Let's verify that this parameter is consistently used across all test files:
✅ Verification successful
All test files consistently use e2e.LocalImages parameter with NobleSpinUpIBC
The verification shows that all calls to NobleSpinUpIBC in the test files (fiat_tf_test.go and conformance_test.go) consistently use the e2e.LocalImages parameter, matching the function signature defined in utils.go. This confirms the change is properly implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of e2e.LocalImages parameter in NobleSpinUpIBC calls
# Expected: All calls should use e2e.LocalImages parameter
# Search for NobleSpinUpIBC function calls
rg -A 2 "NobleSpinUpIBC\(" e2e/
Length of output: 928
e2e/module_check_test.go (1)
31-31: LGTM! The NobleSpinUp parameter update looks correct.
The addition of e2e.LocalImages parameter aligns with the changes across other test files.
upgrade/testnet/store.go (3)
1-14: LGTM: License header is properly formatted.
The Apache 2.0 license header is correctly formatted with up-to-date copyright information.
15-22: LGTM: Package and imports are well-organized.
The package name aligns with the directory structure, and imports are properly organized with clear aliases.
25-27: Verify completeness of module additions.
Let's verify that globalfee is the only module that needs to be added during this upgrade.
✅ Verification successful
Based on the search results, I can now generate a final response:
Confirmed: globalfee is the only module that needs to be added in this upgrade
The verification shows that globalfeetypes.ModuleName is the only new module being added in the store upgrades. The search results reveal:
- The upgrade primarily focuses on migrating GlobalFee legacy params to the new standalone module
- Other module references in the upgrade files are for existing modules (auth, bank, crisis, etc.) and their parameter migrations
- No other new module implementations or additions were found in the upgrade process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other potential module additions in the upgrade
# by looking for module registration patterns and upgrade-related changes
# Look for module registration patterns
echo "Checking for module registrations..."
rg -A 5 "ModuleBasics.*Register|NewModule|modules\s*:=.*{" app/
# Look for upgrade-related changes mentioning modules
echo "Checking for upgrade-related module changes..."
rg -l "upgrade.*module|module.*upgrade" app/ upgrade/
Length of output: 558
Script:
#!/bin/bash
# Let's try a different approach to verify module additions
# Check for module-related changes in upgrade files
echo "Checking upgrade files for module changes..."
rg -A 5 "Module|store.*upgrade" upgrade/
# Look for any new module implementations or configurations
echo "Looking for new module implementations..."
fd -e go . | xargs rg "type Module struct|func New.*Module"
# Check for module initialization or setup
echo "Checking module initialization patterns..."
fd -e go . | xargs rg "ModuleName.*=|RegisterServices|RegisterModule"
Length of output: 6729
e2e/upgrade_test.go (1)
28-56: Verify testnet compatibility.
The PR objectives mention this is specifically for testnet. Let's ensure the test configuration aligns with testnet requirements.
Makefile (1)
90-90: LGTM! Improved build output visibility.
Removing the output redirection provides better visibility into the heighliner build process, which is helpful for debugging and monitoring the build progress.
e2e/cctp_roles_test.go (1)
Line range hint 37-134: LGTM! The test implementations are consistent and well-structured.
The changes to include e2e.LocalImages are consistently applied across all test functions. Each test follows a clear pattern:
- Setup with
NobleSpinUp - Create and fund test users
- Execute role update transactions
- Verify the changes through role queries
e2e/cctp_receive_message_test.go (1)
45-45: Verify consistency of NobleSpinUp usage across test files.
The change to NobleSpinUp appears to be part of a broader standardization effort.
Let's verify that this change has been consistently applied across all test files:
✅ Verification successful
NobleSpinUp usage is consistent across test files
All test files are using the standardized signature with e2e.LocalImages parameter. The only exception is in upgrade_utils.go which uses a genesisImage parameter, but this is expected as it's specifically for upgrade testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of NobleSpinUp across test files
# Expected: All calls should use the new signature with e2e.LocalImages
# Search for all NobleSpinUp function calls
rg -U "NobleSpinUp\([^)]+\)" e2e/
Length of output: 2624
e2e/cctp_deposit_for_burn_with_caller_test.go (2)
41-41: LGTM: Using local images for test reproducibility.
The addition of e2e.LocalImages parameter ensures consistent test environment setup using local container images.
Line range hint 27-190: Enhance security validation in test scenarios.
Consider adding security-focused test cases:
- Validate behavior with malicious/malformed addresses
- Test for potential integer overflow scenarios in amount calculations
- Verify proper handling of cross-chain message tampering
These additions would help ensure the CCTP implementation is robust against security threats.
e2e/cctp_replace_deposit_for_burn_test.go (1)
47-47: Verify the impact of LocalImages parameter.
The addition of e2e.LocalImages parameter aligns with the PR objective of modifying the upgrade handler for testnet. However, we should verify that this change is consistently applied across all e2e tests.
✅ Verification successful
All e2e tests consistently use LocalImages parameter
Based on the search results, all instances of NobleSpinUp and NobleSpinUpIBC across the e2e test suite consistently use e2e.LocalImages as the version parameter. The change in cctp_replace_deposit_for_burn_test.go aligns with this standardization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all e2e tests use LocalImages parameter consistently
# Expected: All NobleSpinUp calls should use e2e.LocalImages
echo "Checking NobleSpinUp usage across e2e tests..."
rg -A 1 "NobleSpinUp" "e2e/"
Length of output: 5002
app.go (3)
37-37: LGTM!
The import statement has been correctly updated to use the testnet upgrade module.
296-302: LGTM!
The upgrade handler has been appropriately updated to use testnet.UpgradeName and testnet.CreateUpgradeHandler with the necessary parameters.
314-315: LGTM!
The store loader is correctly set for the testnet upgrade using testnet.CreateStoreLoader.
e2e/utils.go (4)
30-33: Verify the correctness of the import paths
Ensure that the import paths for the newly added modules are correct and point to the intended repositories. Specifically, confirm that the import path for florintypes (github.com/monerium/module-noble/v2/types) is accurate and consistent with the project's module structure.
42-45: Constants for Docker configurations added appropriately
The addition of ghcrRepo and containerUidGid constants enhances the configurability for Docker image handling.
48-54: Initialization of LocalImages is well-structured
The LocalImages variable is properly defined with the necessary fields for Docker image configuration.
107-110: New interface registrations enhance encoding support
Registering interfaces for halotypes, auratypes, florintypes, and authoritytypes in NobleEncoding expands the types supported by the encoding configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app.go (1)
Line range hint
303-322: Overall implementation looks solid.The upgrade handler implementation is well-structured and follows a clean approach:
- Proper separation of upgrade logic into the testnet package
- Clear integration of the globalfee module
- Appropriate handling of store migrations
The changes align well with the PR objectives of modifying the upgrade handler for the testnet environment.
Consider documenting the upgrade process and any specific testnet considerations in the README or documentation to help other developers understand the upgrade flow.
e2e/upgrade_utils.go (2)
39-45: Add documentation for theChainUpgradestructProviding documentation comments for exported types improves code readability and maintainability.
Consider adding comments to explain the purpose and usage of the
ChainUpgradestruct and its fields. For example:// ChainUpgrade represents an upgrade to be applied to the chain during testing. type ChainUpgrade struct { Image ibc.DockerImage // The Docker image to use for the upgrade UpgradeName string // The name of the upgrade; if empty, a rolling upgrade is assumed Emergency bool // Indicates if the upgrade is an emergency PreUpgrade func(t *testing.T, ctx context.Context, noble *cosmos.CosmosChain, authority ibc.Wallet) // Function to run before the upgrade PostUpgrade func(t *testing.T, ctx context.Context, noble *cosmos.CosmosChain, authority ibc.Wallet) // Function to run after the upgrade }
81-88: Handle errors from node operations with detailed loggingIn the emergency upgrade path, errors from
noble.StopAllNodes(ctx)andnoble.StartAllNodes(ctx)are checked usingrequire.NoError, which will fail the test on error but may not provide sufficient context.Consider adding detailed error handling or logging to provide more information in case of failures. For example:
err := noble.StopAllNodes(ctx) - require.NoError(t, err, "could not stop nodes for emergency upgrade") + if err != nil { + t.Fatalf("Failed to stop all nodes for emergency upgrade: %v", err) + } noble.UpgradeVersion(ctx, client, upgrade.Image.Repository, upgrade.Image.Version) err = noble.StartAllNodes(ctx) - require.NoError(t, err, "could not start nodes for emergency upgrade") + if err != nil { + t.Fatalf("Failed to start all nodes after emergency upgrade: %v", err) + }This provides more detailed error messages, which can aid in debugging if the test fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
e2e/go.modis excluded by!**/*.mode2e/go.sumis excluded by!**/*.sum,!**/*.sumgo.work.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
app.go(4 hunks)e2e/upgrade_utils.go(1 hunks)
🔇 Additional comments (6)
app.go (3)
37-37: LGTM: Import statement correctly updated for testnet upgrade module.
The import statement is properly formatted and consistent with the module versioning scheme.
321-322: Verify store loader implementation.
The store loader configuration is properly integrated with the upgrade process. Let's verify the implementation of the CreateStoreLoader function.
#!/bin/bash
# Description: Verify the CreateStoreLoader function implementation
# Check the CreateStoreLoader function definition and usage
ast-grep --pattern 'func CreateStoreLoader($_) {
$$$
}'
# Look for any store-related migrations or configurations
rg -p "CreateStoreLoader" upgrade/testnet/303-307: Verify upgrade handler configuration and constants.
The upgrade handler is properly configured with the required keepers for the globalfee module integration. However, let's verify the upgrade name constant is properly defined.
✅ Verification successful
Let me try another search strategy to verify the upgrade handler configuration.
Upgrade handler configuration and constants are properly defined
The verification confirms:
- The upgrade name constant
UpgradeNameis defined as "v8.0.0-rc.4" inupgrade/testnet/constants.go - The
CreateUpgradeHandlerfunction inupgrade/testnet/upgrade.goaccepts all the required parameters:- ModuleManager
- Configurator
- InterfaceRegistry
- GlobalFeeKeeper
- ParamsKeeper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the upgrade name constant is properly defined in the testnet package
# and that the CreateUpgradeHandler function accepts the correct parameters.
# Check the upgrade name constant definition
rg -p "UpgradeName.*=.*" upgrade/testnet/
# Verify CreateUpgradeHandler function signature
ast-grep --pattern 'func CreateUpgradeHandler($_) {
$$$
}'
Length of output: 353
Script:
#!/bin/bash
# Search for UpgradeName constant definition
rg -p "var UpgradeName" upgrade/testnet/
# Search for CreateUpgradeHandler function definition with more context
rg -p "func CreateUpgradeHandler" -A 10 upgrade/testnet/
# Search for any upgrade-related constants
rg -p "const.*[Uu]pgrade" upgrade/testnet/
Length of output: 820
e2e/upgrade_utils.go (3)
90-91: Avoid deferring context cancellation inside loops to prevent resource leaks
Deferring timeoutCtxCancel() inside loops causes the deferred calls to accumulate, delaying their execution until the surrounding function returns. This can lead to resource leaks because the contexts remain active longer than necessary.
This issue was previously noted and is still applicable. Modify the code to call timeoutCtxCancel() immediately after it's no longer needed. For example:
At lines 90-91:
timeoutCtx, timeoutCtxCancel := context.WithTimeout(ctx, time.Second*45)
- defer timeoutCtxCancel()
err = testutil.WaitForBlocks(timeoutCtx, int(blocksAfterUpgrade), noble)
require.NoError(t, err, "chain did not produce blocks after emergency upgrade")
+ timeoutCtxCancel()Repeat this pattern for all similar instances to ensure that contexts are properly canceled within the loops.
Also applies to: 114-116, 122-123, 135-136, 178-179, 209-210
173-174: 🛠️ Refactor suggestion
Update error message to match the performed action
The error message in require.NoError states "error submitting software upgrade tx," but the code is executing an upgrade plan query using ExecQuery.
Update the error message to accurately reflect the operation being performed:
stdout, stderr, err := noble.Validators[0].ExecQuery(ctx, "upgrade", "plan")
- require.NoError(t, err, "error submitting software upgrade tx")
+ require.NoError(t, err, "error executing 'upgrade plan' query")Likely invalid or redundant comment.
145-146: Validate the calculated halt height
The halt height is calculated using haltHeight := height + haltHeightDelta. It's important to verify that haltHeight is appropriately set to prevent unintended chain behaviors.
To confirm that the calculated halt height is valid and does not conflict with the current chain parameters, you can run the following script:
This script retrieves the current height and calculates the expected halt height. It also searches the codebase for any occurrences of haltHeight to ensure it's used correctly.
✅ Verification successful
Let me analyze the code further to understand the context of haltHeight calculation.
The halt height calculation is correctly implemented
The code shows that haltHeightDelta is a well-defined constant set to 10 blocks, used in the context of upgrade testing. The calculation haltHeight := height + haltHeightDelta is appropriate as it:
- Sets the upgrade height 10 blocks in the future from the current height
- Is used consistently throughout the test framework for upgrade verification
- Has corresponding validation checks to ensure the upgrade occurs at the correct height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the calculated halt height is appropriate.
# Get the current chain height
current_height=$(noble height)
# Expected halt height based on the delta
expected_halt_height=$((current_height + 10))
echo "Current Height: $current_height"
echo "Expected Halt Height: $expected_halt_height"
# Optional: Check if halt_height is set correctly in the upgrade plan
grep -rnw '.' -e 'haltHeight' --include \*.go
Length of output: 752
Script:
#!/bin/bash
# Let's examine the full context of how haltHeight and haltHeightDelta are used
rg "haltHeightDelta" -B 5 -A 5 ./e2e/upgrade_utils.go
# Also check how this height is used in test scenarios
ast-grep --pattern 'func $_($t *testing.T, $$$) {
$$$
haltHeight := $$$
$$$
}'
Length of output: 694
This PR modify's the upgrade handler to add the
globalfeemodule to testnet.It also adds back a modified and simplified version of the upgrade test that was present in older versions of Noble.
I envision this test being ported over to the main the branch when it becomes needed.