feat: add custom build tags to override certain global variables #3897
Conversation
cmwaters
requested review from
rootulp and
staheri14
and removed request for
a team
WalkthroughWalkthroughThe changes involve modifications to the Dockerfile, Makefile, and various Go source files to introduce new build arguments and constants that enhance configurability for parameters like maximum square size and upgrade height delay. These updates affect how the application is built and how certain operational parameters are managed, allowing for more flexible testing and deployment scenarios. Changes
Assessment against linked issues
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
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: 7
Outside diff range and nitpick comments (15)
pkg/appconsts/overrides.go (3)
3-6: Informative comment block with a minor improvement suggestion.The comment block effectively explains the purpose and implications of the override variables. The warning about network consistency is crucial.
Consider adding a specific reference to the Makefile location or the relevant target for setting these values, to make it easier for developers to locate and use this functionality.
7-10: Well-structured variable declarations with a suggestion for improvement.The variable declarations are appropriate, following Go naming conventions and allowing for compile-time overrides as intended.
Consider adding inline comments or godoc-style comments for each variable, explaining their specific purposes and expected value formats. This would enhance code readability and make it easier for developers to use these overrides correctly.
1-10: Implementation aligns well with PR objectives.The introduction of compile-time overrides using string variables and LD flags successfully addresses the need for customizable upper bounds as outlined in issue #3603. This approach provides the flexibility required for performance testing scenarios while maintaining default values for normal operation.
To further enhance this implementation:
- Consider adding a mechanism to validate the override values at runtime to ensure they are within acceptable ranges.
- Document the process of setting these overrides in the project's main documentation, including examples of usage in different scenarios (e.g., performance testing, development environments).
- Implement logging or a startup message that indicates when these overrides are in use, to prevent accidental use in production environments.
pkg/appconsts/versioned_consts.go (1)
Line range hint
1-33: Overall assessment: Functionality implemented with room for improvementsThe changes successfully implement the ability to override the square size upper bound at compile time, aligning with the PR objectives. However, there are several areas for improvement:
- Add more robust validation for the override value.
- Provide more informative error messages.
- Consider updating the function signature or documenting the unused parameter.
- Ensure proper definition and documentation of the
OverrideSquareSizeUpperBoundStrvariable.These improvements will enhance the reliability and maintainability of the code. Once addressed, the implementation will be more robust and aligned with best practices.
pkg/appconsts/global_consts.go (2)
30-33: LGTM: Well-defined constant with clear explanation.The
DefaultUpgradeHeightDelayconstant is well-defined and aligns with the PR objectives. The comment clearly explains its purpose and calculation.Consider adding the resulting value (50,400) to the comment for quick reference:
- DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks. + DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks
Line range hint
1-64: Overall assessment: Implementation aligns with objectives, but needs refinement.The changes successfully introduce the ability to customize the upgrade height delay, which aligns with the PR objectives. However, there are areas for improvement:
- Better documentation for the
OverrideUpgradeHeightDelayStrvariable.- Enhanced error handling in the
UpgradeHeightDelay()function.- Input validation to ensure non-negative values for the delay.
Addressing these points will improve the robustness and maintainability of the code.
Dockerfile (2)
11-13: LGTM! Consider a minor improvement to the comment.The addition of the MAX_SQUARE_SIZE build argument is well-implemented and aligns with the PR objectives. It provides the desired flexibility for customizing the maximum square size during the build process.
Consider updating the comment to include the default value for MAX_SQUARE_SIZE, if one exists. This would provide more context for users. For example:
-# Use build args to override the maxuimum square size of the docker image i.e. +# Use build args to override the maximum square size of the docker image (default: X) i.e.Also, there's a typo in "maxuimum" which should be corrected to "maximum".
14-16: LGTM! Consider a minor improvement to the comment.The addition of the UPGRADE_HEIGHT_DELAY build argument is well-implemented and aligns with the PR objectives. It provides the desired flexibility for customizing the upgrade height delay during the build process.
Consider updating the comment to include the default value for UPGRADE_HEIGHT_DELAY, if one exists. This would provide more context for users. For example:
-# Use build args to override the upgrade height delay of the docker image i.e. +# Use build args to override the upgrade height delay of the docker image (default: Y) i.e.app/module/configurator_test.go (1)
Line range hint
1-114: Consider updating test cases for comprehensive coverage.While the change to
NewKeeperhas been implemented, the test cases in this file haven't been updated to thoroughly test the new parameter's functionality. To ensure robust testing:
- Add test cases that use different values for the new parameter.
- Verify that the
Keeperbehaves correctly with various inputs, including edge cases.- If applicable, add tests that check how this parameter affects the upper bounds mentioned in the PR objectives.
x/signal/keeper.go (2)
44-47: LGTM! Consider enhancing the comment slightly.The addition of the
upgradeHeightDelayBlocksfield to theKeeperstruct is well-implemented and aligns with the PR objectives. It allows for configurable upgrade height delays, which enhances flexibility for different network configurations and testing scenarios.Consider slightly modifying the comment to be more specific:
- // upgradeHeightDelayBlocks is the number of blocks after a quorum has been - // reached that the chain should upgrade to the new version + // upgradeHeightDelayBlocks is the number of blocks to wait after a quorum has been + // reached before the chain upgrades to the new versionThis minor change makes the purpose of the field even clearer.
112-112: LGTM! Consider adding a comment for clarity.The modification to use
k.upgradeHeightDelayBlocksin theTryUpgrademethod is correct and aligns with the PR objectives. This change allows for a configurable delay in the upgrade process, enhancing the flexibility of the system.Consider adding a brief comment to explain the calculation:
- UpgradeHeight: sdkCtx.BlockHeader().Height + k.upgradeHeightDelayBlocks, + // Set the upgrade height to the current height plus the configured delay + UpgradeHeight: sdkCtx.BlockHeader().Height + k.upgradeHeightDelayBlocks,This comment would provide additional clarity about the purpose of this calculation.
Makefile (1)
11-14: LGTM! Consider adding documentation for these override variables.The addition of
OVERRIDE_MAX_SQUARE_SIZEandOVERRIDE_UPGRADE_HEIGHT_DELAYvariables aligns well with the PR objectives. They provide a flexible way to customize upper bounds at compile time.Consider adding comments explaining the purpose and expected values for these variables to improve maintainability.
x/signal/keeper_test.go (3)
72-72: NewKeeper parameter update looks goodThe addition of
appconsts.DefaultUpgradeHeightDelayas a parameter toNewKeeperis consistent with the updated function signature. This change aligns with the PR objectives of using build tags for customizable limits.Consider adding a comment explaining the significance of
appconsts.DefaultUpgradeHeightDelayin this context for better code readability.
455-455: NewKeeper parameter update in setup function is correctThe addition of
appconsts.DefaultUpgradeHeightDelayas a parameter toNewKeeperin the setup function is consistent with the updated function signature and ensures all tests use the correct upgrade height delay.For consistency, consider extracting
appconsts.DefaultUpgradeHeightDelayinto a local variable at the beginning of thesetupfunction, especially if it's used in multiple places within the function.
Line range hint
1-497: Overall changes look good and align with PR objectivesThe changes in this file consistently update the usage of upgrade height delay to use
appconsts.DefaultUpgradeHeightDelay. This aligns with the PR objectives of using build tags for customizable limits. The tests have been appropriately updated to reflect the newNewKeeperfunction signature and the use of the new constant.Consider adding a test case that specifically verifies the behavior when a custom upgrade height delay is used, to ensure the flexibility introduced by this change is properly tested.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- Dockerfile (2 hunks)
- Makefile (2 hunks)
- app/app.go (2 hunks)
- app/module/configurator_test.go (1 hunks)
- app/test/upgrade_test.go (3 hunks)
- pkg/appconsts/global_consts.go (3 hunks)
- pkg/appconsts/overrides.go (1 hunks)
- pkg/appconsts/versioned_consts.go (2 hunks)
- x/signal/integration_test.go (2 hunks)
- x/signal/keeper.go (2 hunks)
- x/signal/keeper_test.go (5 hunks)
Additional comments not posted (19)
pkg/appconsts/overrides.go (1)
1-1: LGTM: Package declaration is appropriate.The package name
appconstsis well-suited for application constants and overrides, aligning with the file's purpose and location.pkg/appconsts/versioned_consts.go (1)
4-5: LGTM: Import statement added correctlyThe addition of the "strconv" import is necessary for the new functionality and is correctly placed.
pkg/appconsts/global_consts.go (1)
4-5: LGTM: Import addition is appropriate.The addition of the "strconv" package import is necessary for the new
UpgradeHeightDelay()function to parse string values to int64.Dockerfile (2)
Line range hint
1-83: Overall, the changes look good and align with the PR objectives.The modifications to the Dockerfile successfully implement the ability to customize the maximum square size and upgrade height delay at build time. This enhancement aligns well with the PR objectives and the linked issue #3603, providing the desired flexibility for performance testing scenarios.
Key points:
- Two new build arguments (MAX_SQUARE_SIZE and UPGRADE_HEIGHT_DELAY) have been added.
- Clear comments explain how to use these build arguments.
- The build arguments are correctly used to set environment variables during the build process.
These changes should allow developers to define certain limits at compile time without needing extensive modifications to the codebase, as intended.
37-38: LGTM! Verify the usage of these environment variables in the build process.The implementation correctly uses the new build arguments to set environment variables during the build process. This approach aligns with the PR objectives and allows for the desired customization of global variables.
To ensure that these environment variables are being used correctly in the build process, please run the following script:
This will help confirm that the build process is correctly utilizing these environment variables to override the global variables as intended.
Verification successful
Verified: Environment variables are correctly used in the build process.
The environment variables
OVERRIDE_MAX_SQUARE_SIZEandOVERRIDE_UPGRADE_HEIGHT_DELAYare appropriately utilized in the Makefile, ensuring they correctly override the global variables as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of OVERRIDE_MAX_SQUARE_SIZE and OVERRIDE_UPGRADE_HEIGHT_DELAY in the build process. # Test: Search for the usage of these environment variables in the Makefile or any build scripts. rg --type make --type shell 'OVERRIDE_MAX_SQUARE_SIZE|OVERRIDE_UPGRADE_HEIGHT_DELAY'Length of output: 116
Script:
#!/bin/bash # Description: Verify the usage of OVERRIDE_MAX_SQUARE_SIZE and OVERRIDE_UPGRADE_HEIGHT_DELAY in the build process. # Test: Search for the usage of these environment variables in the Makefile or any build scripts. rg --type make --type sh 'OVERRIDE_MAX_SQUARE_SIZE|OVERRIDE_UPGRADE_HEIGHT_DELAY'Length of output: 427
x/signal/integration_test.go (3)
7-7: LGTM: Import change aligns with constant usageThe addition of the
appconstsimport is consistent with the usage ofappconsts.DefaultUpgradeHeightDelaylater in the file. This change suggests a refactoring of constant definitions, moving them from thesignalpackage to theappconstspackage, which aligns with the PR objectives of making certain constants more customizable.
80-80: LGTM: Constant usage aligns with PR objectivesThe change from
signal.DefaultUpgradeHeightDelaytoappconsts.DefaultUpgradeHeightDelayis consistent with the PR objectives. This modification allows for easier customization of the upgrade height delay, which is beneficial for performance testing scenarios as mentioned in issue #3603.
Line range hint
1-85: Verify constant value and consider adding a commentThe changes in this file align well with the PR objectives, making the upgrade height delay more customizable for testing purposes. However, to ensure consistent behavior:
- Please verify that the default value of
appconsts.DefaultUpgradeHeightDelaymatches the previous value ofsignal.DefaultUpgradeHeightDelay.- Consider adding a comment near the usage of
appconsts.DefaultUpgradeHeightDelayto explain that this value can be customized at compile-time, which would help other developers understand the flexibility introduced by this change.To verify the constant value, you can run:
x/signal/keeper.go (1)
Line range hint
1-283: Overall assessment: Changes successfully implement configurable upgrade height delayThe modifications in this file effectively address the PR objectives by introducing a configurable upgrade height delay. The changes are well-integrated into the existing code structure and maintain consistency throughout the file. The new
upgradeHeightDelayBlocksfield in theKeeperstruct, along with the corresponding updates in theNewKeeperfunction andTryUpgrademethod, provide the desired flexibility for setting custom upper bounds to block size.Key points:
- The new
upgradeHeightDelayBlocksfield allows for customizable delay configuration.- The
NewKeeperfunction now accepts this delay as a parameter, enabling easy configuration during keeper initialization.- The
TryUpgrademethod utilizes the configured delay to calculate the appropriate upgrade height.These changes align well with the goal of enhancing flexibility for performance testing scenarios and allowing developers to define certain limits at compile time.
To ensure a smooth integration of these changes:
- Verify that all calls to
NewKeeperin the codebase are updated with the appropriateupgradeHeightDelayBlocksvalue.- Consider adding unit tests to validate the behavior of the
TryUpgrademethod with differentupgradeHeightDelayBlocksvalues.- Update any relevant documentation to reflect the new configurable upgrade height delay feature.
app/test/upgrade_test.go (3)
110-110: LGTM: Consistent use of new constant in EndBlock request.The change to use
appconsts.DefaultUpgradeHeightDelayin the EndBlock request is consistent with the previous modification. This ensures that the test uses the same constant throughout for the upgrade height delay.
Line range hint
101-132: Summary: Consistent updates to use new constant source for upgrade height delay.The changes in this file consistently replace
signal.DefaultUpgradeHeightDelaywithappconsts.DefaultUpgradeHeightDelay. This aligns with the PR objectives of using LD flags for customizable upper bounds. The test logic remains intact, with only the constant source being updated.Key points:
- The for loop condition now uses the new constant (line 101).
- The EndBlock request height uses the new constant (line 110).
- The BeginBlock request header height uses the new constant (line 132).
These changes improve consistency and maintainability of the upgrade tests. Ensure that the new constant value matches the intended upgrade delay to maintain the test's effectiveness.
101-101: LGTM: Constant source updated for upgrade height delay.The change from
signal.DefaultUpgradeHeightDelaytoappconsts.DefaultUpgradeHeightDelayaligns the test with the new constant source. This update is consistent with the PR objectives.Please verify that this change doesn't alter the expected test behavior. Run the following command to check if the constant value has changed:
Makefile (2)
40-40: Please clarify the removal ofgo.sumdependency.The
installtarget no longer depends ongo.sum. While this simplifies the installation process, it may affect build reproducibility.Could you explain the rationale behind this change? If it's intentional, consider updating the PR description to include this modification.
To assess the potential impact, please run the following commands and review the output:
#!/bin/bash # Check if go.sum is being used in other targets grep -n "go.sum" Makefile # Verify if go.mod and go.sum are in sync go mod tidy git diff go.sum echo "Please review the output to ensure that removing go.sum dependency doesn't introduce inconsistencies."
21-22: LGTM! Verify package paths for correctness.The addition of new flags to
ldflagscorrectly incorporates the override variables into the build process, allowing for compile-time customization as intended.Please verify that the package paths are correct:
Verification successful
Verification Successful: Package paths and variables are correctly defined.
The variables
OverrideSquareSizeUpperBoundStrandOverrideUpgradeHeightDelayStrare properly defined ingit.luolix.top/celestiaorg/celestia-app/v3/pkg/appconsts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the packages and variables referenced in ldflags # Check for the appconsts package if ! go list github.com/celestiaorg/celestia-app/v3/pkg/appconsts &> /dev/null; then echo "Package github.com/celestiaorg/celestia-app/v3/pkg/appconsts not found" exit 1 fi # Check for the specific variables in the package go doc github.com/celestiaorg/celestia-app/v3/pkg/appconsts | grep -E "OverrideSquareSizeUpperBoundStr|OverrideUpgradeHeightDelayStr" echo "Package and variable checks completed"Length of output: 438
Script:
#!/bin/bash # Verify that OverrideSquareSizeUpperBoundStr and OverrideUpgradeHeightDelayStr are defined in pkg/appconsts # Search for OverrideSquareSizeUpperBoundStr in pkg/appconsts rg 'OverrideSquareSizeUpperBoundStr' pkg/appconsts -A 2 # Search for OverrideUpgradeHeightDelayStr in pkg/appconsts rg 'OverrideUpgradeHeightDelayStr' pkg/appconsts -A 2 echo "Variable presence checks completed"Length of output: 1275
x/signal/keeper_test.go (3)
15-15: Import addition looks goodThe addition of the
appconstspackage import is consistent with its usage in the code changes.
186-186: Context block height update is correctThe use of
appconsts.DefaultUpgradeHeightDelayfor updating the context block height is consistent with the PR objectives and the overall changes in the file.
429-429: Upgrade height assertion update is correctThe assertion update to use
appconsts.DefaultUpgradeHeightDelayfor theUpgradeHeightis consistent with the overall changes and ensures the correct upgrade height is set.app/app.go (2)
13-13: LGTM: New import added correctlyThe new import for
appconstsis correctly added and will be used for theUpgradeHeightDelay()function in theSignalKeeperinitialization.
283-283: LGTM: SignalKeeper initialization updated with UpgradeHeightDelayThe
SignalKeeperinitialization has been correctly updated to include theappconsts.UpgradeHeightDelay()parameter. This change aligns with the PR objectives of adding custom build tags to override certain global variables.Please verify the impact of this change on the upgrade process:
Verification successful
Verified: UpgradeHeightDelay integration is consistent and impact-free
The
SignalKeeperinitialization correctly incorporatesappconsts.UpgradeHeightDelay(). All existing usages and related tests have been reviewed and remain consistent with this change. No adverse impacts detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of UpgradeHeightDelay and potential impacts # Search for other usages of UpgradeHeightDelay echo "Searching for other usages of UpgradeHeightDelay:" rg "UpgradeHeightDelay" # Check for any upgrade-related tests that might need updating echo "Checking for upgrade-related tests:" rg -i "upgrade.*test"Length of output: 10958
| if OverrideSquareSizeUpperBoundStr != "" { | ||
| parsedValue, err := strconv.Atoi(OverrideSquareSizeUpperBoundStr) | ||
| if err != nil { | ||
| panic("Invalid OverrideSquareSizeUpperBoundStr value") | ||
| } | ||
| return parsedValue | ||
| } |
There was a problem hiding this comment.
Consider updating the function signature
The function still takes an unused uint64 parameter. Consider updating the function signature to reflect that this parameter is no longer used, or document why it's kept for backward compatibility.
If the parameter is no longer needed, update the function signature:
-func SquareSizeUpperBound(_ uint64) int {
+func SquareSizeUpperBound() int {If it's kept for backward compatibility, add a comment explaining this:
// SquareSizeUpperBound imposes an upper bound on the max effective square size.
// The uint64 parameter is kept for backward compatibility but is no longer used.
func SquareSizeUpperBound(_ uint64) int {
// ... (rest of the function)
}Implement additional validation for the override value
While the implementation allows for customizable upper bounds as intended, consider adding validation to ensure the parsed value is positive and within a reasonable range. This will prevent potential issues with extremely large or negative values.
Consider modifying the function as follows:
if OverrideSquareSizeUpperBoundStr != "" {
parsedValue, err := strconv.Atoi(OverrideSquareSizeUpperBoundStr)
- if err != nil {
- panic("Invalid OverrideSquareSizeUpperBoundStr value")
+ if err != nil || parsedValue <= 0 {
+ panic(fmt.Sprintf("Invalid OverrideSquareSizeUpperBoundStr value: %s. Must be a positive integer.", OverrideSquareSizeUpperBoundStr))
}
+ // Add an upper limit check if necessary, e.g.:
+ // if parsedValue > someMaxAllowedValue {
+ // panic(fmt.Sprintf("OverrideSquareSizeUpperBoundStr value too large: %d. Maximum allowed: %d", parsedValue, someMaxAllowedValue))
+ // }
return parsedValue
}This change will provide more informative error messages and ensure the override value is valid.
Committable suggestion was skipped due to low confidence.
| @@ -22,6 +24,13 @@ | |||
|
|
|||
| // SquareSizeUpperBound imposes an upper bound on the max effective square size. | |||
| func SquareSizeUpperBound(_ uint64) int { | |||
| if OverrideSquareSizeUpperBoundStr != "" { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Issue Found: OverrideSquareSizeUpperBoundStr is Defined as a Struct Field but Used as a Global Variable
The variable OverrideSquareSizeUpperBoundStr is defined as a field within a struct in pkg/appconsts/overrides.go but is incorrectly used as a standalone global variable in pkg/appconsts/versioned_consts.go. This misuse can lead to undefined behavior or runtime errors.
- Location of Misuse:
pkg/appconsts/versioned_consts.go- Line: 27
- Usage:
if OverrideSquareSizeUpperBoundStr != "" {
Recommended Actions:
- Access
OverrideSquareSizeUpperBoundStrthrough its parent struct instance inversioned_consts.go. - If a global variable is intended, properly define and document
OverrideSquareSizeUpperBoundStras a global variable or constant.
Analysis chain
Verify the definition of OverrideSquareSizeUpperBoundStr
The global variable OverrideSquareSizeUpperBoundStr is used here but not defined in this file. Ensure it's properly defined and documented in the appropriate location.
Run the following script to check for the definition of OverrideSquareSizeUpperBoundStr:
If the variable is not found, it needs to be defined and documented properly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition of OverrideSquareSizeUpperBoundStr
# Test: Look for the variable definition
rg --type go -A 5 $'var OverrideSquareSizeUpperBoundStr'
# If not found as a var, check for const definition
rg --type go -A 5 $'const OverrideSquareSizeUpperBoundStr'
# If still not found, search for any occurrence to see if it's defined differently
rg --type go $'OverrideSquareSizeUpperBoundStr'
Length of output: 503
| func UpgradeHeightDelay() int64 { | ||
| if OverrideUpgradeHeightDelayStr != "" { | ||
| parsedValue, err := strconv.ParseInt(OverrideUpgradeHeightDelayStr, 10, 64) | ||
| if err != nil { | ||
| panic("Invalid OverrideUpgradeHeightDelayStr value") | ||
| } | ||
| return parsedValue | ||
| } | ||
| return DefaultUpgradeHeightDelay | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Function implementation needs improvement.
The UpgradeHeightDelay() function aligns with the PR objectives, and it has been confirmed that OverrideUpgradeHeightDelayStr is properly defined in pkg/appconsts/overrides.go. However, there are still some concerns:
- Panicking on invalid input might not be the best approach for error handling.
- There's no validation for negative values, which wouldn't make sense for a delay.
Consider the following improvements:
- Use error returning instead of panicking.
- Add validation for non-negative values.
Analysis chain
Function implementation needs improvement.
The UpgradeHeightDelay() function aligns with the PR objectives, but there are some concerns:
- The
OverrideUpgradeHeightDelayStrvariable is not defined in this file. Ensure it's properly declared and documented. - Panicking on invalid input might not be the best approach for error handling.
- There's no validation for negative values, which wouldn't make sense for a delay.
Consider the following improvements:
- Add a comment explaining where
OverrideUpgradeHeightDelayStris defined. - Use error returning instead of panicking.
- Add validation for non-negative values.
Here's a suggested implementation:
var ErrInvalidUpgradeHeightDelay = errors.New("invalid UpgradeHeightDelay value")
func UpgradeHeightDelay() (int64, error) {
if OverrideUpgradeHeightDelayStr != "" {
parsedValue, err := strconv.ParseInt(OverrideUpgradeHeightDelayStr, 10, 64)
if err != nil {
return 0, fmt.Errorf("%w: %s", ErrInvalidUpgradeHeightDelay, err)
}
if parsedValue < 0 {
return 0, fmt.Errorf("%w: value must be non-negative", ErrInvalidUpgradeHeightDelay)
}
return parsedValue, nil
}
return DefaultUpgradeHeightDelay, nil
}This approach provides better error handling and ensures the returned value is always non-negative.
To ensure OverrideUpgradeHeightDelayStr is properly defined, run:
| @@ -37,7 +37,7 @@ func TestConfigurator(t *testing.T) { | |||
| stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db) | |||
| require.NoError(t, stateStore.LoadLatestVersion()) | |||
|
|
|||
| keeper := signal.NewKeeper(config.Codec, storeKey, nil) | |||
| keeper := signal.NewKeeper(config.Codec, storeKey, nil, 0) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent NewKeeper Function Usage Detected
The recent addition of a new parameter to the NewKeeper function in configurator_test.go is not reflected consistently across the codebase. Several instances of NewKeeper are still being called without the new parameter, which could lead to potential issues.
- Affected Files:
- x/tokenfilter/keeper.go
- x/signal/keeper_test.go
- x/signal/keeper.go
- x/mint/keeper/keeper.go
- x/blobstream/keeper/keeper.go
- test/pfm/simapp.go
- test/util/common.go
- x/blob/keeper/keeper_test.go
- x/blob/keeper/keeper.go
- app/app.go
- app/ante/min_fee_test.go
Please ensure all NewKeeper function calls are updated accordingly to maintain consistency.
Analysis chain
LGTM, but consider adding more test cases.
The addition of the new parameter to NewKeeper aligns with the changes mentioned in the AI summary. This change likely relates to the PR objectives of adding customizable upper bounds.
Consider adding more test cases that use different values for this new parameter to ensure the Keeper behaves correctly with various inputs.
Let's verify if this change is consistent across the codebase:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all NewKeeper function calls in the codebase
rg --type go 'NewKeeper\(' -A 3
Length of output: 12876
| } | ||
|
|
||
| // NewKeeper returns a signal keeper. | ||
| func NewKeeper( | ||
| binaryCodec codec.BinaryCodec, | ||
| storeKey storetypes.StoreKey, | ||
| stakingKeeper StakingKeeper, | ||
| upgradeHeightDelayBlocks int64, |
There was a problem hiding this comment.
Tip
Codebase Verification
Missing upgradeHeightDelayBlocks Parameter in NewKeeper Calls
Several calls to NewKeeper do not include the required upgradeHeightDelayBlocks parameter. Please update the following instances to include this new parameter:
x/tokenfilter/keeper.go:func NewKeeper(wrapper porttypes.ICS4Wrapper) Keeper { ... }x/blobstream/keeper.go:func NewKeeper(cdc codec.BinaryCodec, storeKey storetypes.StoreKey, paramSpace paramtypes.Subspace, stakingKeeper StakingKeeper) *Keeper { ... }
Analysis chain
LGTM! Verify impact on keeper initialization.
The changes to the NewKeeper function correctly incorporate the new upgradeHeightDelayBlocks parameter. This modification allows for flexible configuration of the upgrade delay at the time of keeper creation, which aligns well with the PR objectives.
To ensure that these changes don't break existing code, please verify all calls to NewKeeper in the codebase. Run the following script to check for any instances that need to be updated:
Make sure to update all instances of NewKeeper calls with the appropriate upgradeHeightDelayBlocks value.
Also applies to: 61-61
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to NewKeeper and verify they include the new parameter
# Search for NewKeeper calls
rg --type go "NewKeeper\s*\(" -A 10
Length of output: 23733
| @@ -130,7 +129,7 @@ func TestAppUpgradeV3(t *testing.T) { | |||
| _ = testApp.BeginBlock(abci.RequestBeginBlock{ | |||
| Header: tmproto.Header{ | |||
| ChainID: genesis.ChainID, | |||
| Height: initialHeight + signal.DefaultUpgradeHeightDelay, | |||
| Height: initialHeight + appconsts.DefaultUpgradeHeightDelay, | |||
There was a problem hiding this comment.
LGTM: Consistent use of new constant in BeginBlock request.
The change to use appconsts.DefaultUpgradeHeightDelay in the BeginBlock request header is consistent with the previous modifications. This ensures that the test uses the same constant throughout for the upgrade height.
Consider refactoring the upgrade height calculation into a separate variable to improve code maintainability. For example:
+upgradeHeight := initialHeight + appconsts.DefaultUpgradeHeightDelay
_ = testApp.BeginBlock(abci.RequestBeginBlock{
Header: tmproto.Header{
ChainID: genesis.ChainID,
- Height: initialHeight + appconsts.DefaultUpgradeHeightDelay,
+ Height: upgradeHeight,
Version: tmversion.Consensus{App: 3},
},
})This refactoring would make it easier to update the upgrade height calculation in the future if needed.
Committable suggestion was skipped due to low confidence.
Closes: #3603
Technically this uses LD flags not build flags