refactor: override the upgrade height for chain-id test to 3
#4065
Conversation
rootulp
requested review from
cmwaters and
evan-forbes
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves several modifications across multiple files, primarily focused on the removal of the 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: 0
🧹 Outside diff range and nitpick comments (4)
pkg/appconsts/versioned_consts.go (1)
83-84: LGTM! Consider minor improvements for maintainability.The implementation correctly addresses the PR objective by setting a fixed upgrade height delay of 3 blocks for the test chain-id.
Consider these improvements:
+ // TestChainID is the chain ID used for testing environments + const TestChainID = "test" + // TestUpgradeHeightDelay is the number of blocks to wait before upgrading in test environments + const TestUpgradeHeightDelay = 3 + func UpgradeHeightDelay(chainID string, v uint64) int64 { - if chainID == "test" { - return 3 + if chainID == TestChainID { + return TestUpgradeHeightDelaypkg/appconsts/versioned_consts_test.go (1)
121-132: Differentiate the test case namesThe two test cases have identical names which could cause confusion when a test fails. Consider making the names more specific to each version being tested.
Apply this diff to make the test names more distinct:
- name: "the upgrade delay for chainID 'test' should be 3 regardless of the version", + name: "the upgrade delay for chainID 'test' should be 3 for version 3", ... - name: "the upgrade delay for chainID 'test' should be 3 regardless of the version", + name: "the upgrade delay for chainID 'test' should be 3 for version 4",app/test/upgrade_test.go (2)
258-258: LGTM, but consider adding explicit verification.The addition of chain ID "test" aligns with the PR objectives. However, consider adding explicit assertions to verify that the upgrade height delay is correctly overridden to 3 for this chain ID.
Add assertions like:
genesis := genesis.NewDefaultGenesis(). WithChainID("test"). WithValidators(genesis.NewDefaultValidator(testnode.DefaultValidatorAccountName)). WithConsensusParams(app.DefaultInitialConsensusParams()) +// Verify upgrade height override for chain-id "test" +require.Equal(t, int64(3), appconsts.UpgradeHeightDelay("test", v2.Version))
Line range hint
89-92: Make upgrade height delay expectations explicit.The test implicitly validates the upgrade height delay through the loop, but it would be clearer if the expectations were explicit.
Consider adding:
+expectedDelay := int64(3) // explicit expectation for chain-id "test" +require.Equal(t, expectedDelay, appconsts.UpgradeHeightDelay(testApp.GetChainID(), v2.Version)) for height := initialHeight; height < initialHeight+appconsts.UpgradeHeightDelay(testApp.GetChainID(), v2.Version); height++ {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
Makefile(1 hunks)app/test/upgrade_test.go(1 hunks)pkg/appconsts/overrides.go(0 hunks)pkg/appconsts/versioned_consts.go(1 hunks)pkg/appconsts/versioned_consts_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/appconsts/overrides.go
🔇 Additional comments (3)
pkg/appconsts/versioned_consts.go (1)
83-84: Verify chain-id usage in tests and local development.
Let's ensure this change only affects test environments and doesn't impact production chains.
✅ Verification successful
Chain-id "test" usage is properly isolated to test environments
The verification confirms that:
- The "test" chain-id is only used in test files (
*_test.go) and local development scripts - The production chain logic is separate and handles other chain IDs (like "arabica") differently
- The change only affects the upgrade height delay for test environments, returning a fixed value of 3 blocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of chain-ids in tests and configuration files
# Expected: Only test-related files should use chain-id "test"
echo "Checking for chain-id usage in test files:"
rg -g '**/*_test.go' '"test"' -C 2
echo "Checking for chain-id usage in configuration and script files:"
rg -g '*.{sh,json,toml,yaml,yml}' '"test"' -C 2
echo "Checking for other chain-ids to ensure they're not affected:"
rg 'chainID == "(?!test)' -C 2
Length of output: 10876
Script:
#!/bin/bash
# Let's check for other chain IDs with a simpler pattern
echo "Checking for chainID comparisons in the codebase:"
rg 'chainID ==' -C 2
# Let's also check the specific function implementation
echo -e "\nChecking the UpgradeHeightDelay implementation:"
ast-grep --pattern 'func UpgradeHeightDelay($_, $_) $$$'
Length of output: 2259
pkg/appconsts/versioned_consts_test.go (1)
121-132: LGTM! The test cases effectively verify the new upgrade height delay behavior.
The added test cases properly verify that:
- The chain-id "test" consistently returns an upgrade height delay of 3
- This behavior is version-independent (tested with versions 3 and 4)
This aligns well with the PR objective of providing shorter upgrade delays for testing environments.
Makefile (1)
37-37: LGTM! Clean removal of the upgrade height delay override.
The removal of the OverrideUpgradeHeightDelayStr linker flag aligns with the PR objective to simplify upgrade height overrides by removing one of the multiple override methods.
Let's verify no other references to the removed variable exist:
✅ Verification successful
Verified: No remaining references to the removed variable in the codebase
The search results confirm that there are no remaining references to OverrideUpgradeHeightDelayStr in the codebase. The only matches found are:
- A documentation file (
adr-018-network-upgrades.md) discussing the general concept of upgrade height delays - A comment in Dockerfile about build args, which is unrelated to the specific variable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the removed variable
# Test: Expect no matches for OverrideUpgradeHeightDelayStr
# Search in all files
rg -i "OverrideUpgradeHeightDelayStr"
# Search specifically in go files for any upgrade height delay related code
rg "upgrade.*height.*delay"
Length of output: 1702
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/architecture/adr-015-namespace-id-size.md (1)
53-53: Fix grammar issues in the sentenceThe sentence has two grammar issues:
- Subject-verb agreement: "it fail" should be "it fails"
- Number agreement: "this criteria" should be "this criterion" (since referring to Criteria 1)
-The namespace ID must provide at least 72 bits of randomness to satisfy criteria 1. Since an 8 byte namespace ID can only provide 64 bits of randomness, it fail to meet this criteria. +The namespace ID must provide at least 72 bits of randomness to satisfy criteria 1. Since an 8 byte namespace ID can only provide 64 bits of randomness, it fails to meet this criterion.🧰 Tools
🪛 LanguageTool
[uncategorized] ~53-~53: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... only provide 64 bits of randomness, it fail to meet this criteria. | Namespace ID ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[grammar] ~53-~53: The singular determiner ‘this’ may not agree with the plural noun ‘criteria’. Did you mean “these”?
Context: ... 64 bits of randomness, it fail to meet this criteria. | Namespace ID size (bytes) ...(THIS_NNS)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
docs/architecture/adr-015-namespace-id-size.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/architecture/adr-015-namespace-id-size.md
[uncategorized] ~53-~53: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... only provide 64 bits of randomness, it fail to meet this criteria. | Namespace ID ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[grammar] ~53-~53: The singular determiner ‘this’ may not agree with the plural noun ‘criteria’. Did you mean “these”?
Context: ... 64 bits of randomness, it fail to meet this criteria. | Namespace ID size (bytes) ...
(THIS_NNS)
| panic("Invalid OverrideUpgradeHeightDelayStr value") | ||
| } | ||
| return parsedValue | ||
| if chainID == "test" { |
There was a problem hiding this comment.
nit can we use a constant name (that can also be used in tests)
Closes #4053
Description
testto 3 so that there are still a few blocks of delay in testsOverrideUpgradeHeightDelayStrbecause it was confusing to have multiple ways to override the upgrade height (1) chain-id and (2) via ENV variables