feat: hardcoded block time related values changes#3932
Conversation
skosito
added
TSS_MIGRATION_TESTS
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces a new migration for the emissions module, incrementing its consensus version and adding a migration routine for version 5 to 6. Several block time-related constants are adjusted for improved timing precision. The test suite is updated to accommodate these timing changes. Documentation is updated to reflect the new feature. Changes
Sequence Diagram(s)sequenceDiagram
participant AppModule
participant Migrator
participant v6Migration
participant EmissionsKeeper
participant Store
AppModule->>Migrator: Register Migrate5to6 for version 5
Migrator->>v6Migration: Call MigrateStore(ctx, emissionsKeeper)
v6Migration->>EmissionsKeeper: GetParams(ctx)
EmissionsKeeper->>v6Migration: Return Params
v6Migration->>EmissionsKeeper: SetParams(ctx, updatedParams)
EmissionsKeeper->>Store: Save updated Params
v6Migration->>Migrator: Return success/error
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3932 +/- ##
========================================
Coverage 64.84% 64.85%
========================================
Files 471 472 +1
Lines 34434 34446 +12
========================================
+ Hits 22330 22341 +11
- Misses 11074 11075 +1
Partials 1030 1030
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
x/emissions/migrations/v6/migrate_test.go (1)
13-51: Comprehensive test coverage with one gap.The test effectively validates that only the intended parameters are updated while others remain unchanged. The expected values align correctly with the migration logic.
Consider adding a test case for the error scenario when parameters are not found:
+ t.Run("return error when params not found", func(t *testing.T) { + // Arrange + k, ctx, _, _ := keepertest.EmissionsKeeper(t) + // Don't set params to simulate missing params scenario + + // Act + err := v6.MigrateStore(ctx, k) + + // Assert + require.Error(t, err) + require.Contains(t, err.Error(), "emission params not found") + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetacored/root.go(1 hunks)e2e/e2etests/test_bitcoin_donation.go(1 hunks)pkg/constant/constant.go(1 hunks)x/emissions/keeper/migrator.go(2 hunks)x/emissions/migrations/v6/migrate.go(1 hunks)x/emissions/migrations/v6/migrate_test.go(1 hunks)x/emissions/module.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_donation.gopkg/constant/constant.gocmd/zetacored/root.gox/emissions/keeper/migrator.gox/emissions/module.gox/emissions/migrations/v6/migrate_test.gox/emissions/migrations/v6/migrate.go
🧬 Code Graph Analysis (3)
e2e/e2etests/test_bitcoin_donation.go (1)
pkg/constant/constant.go (1)
ZetaBlockTime(8-8)
x/emissions/migrations/v6/migrate_test.go (4)
x/emissions/migrations/v6/migrate.go (2)
EmissionsKeeper(13-16)MigrateStore(20-35)x/emissions/types/params.go (1)
DefaultParams(24-26)x/emissions/types/keys.go (2)
BallotMaturityBlocks(54-54)ObserverSlashAmount(50-50)x/observer/migrations/v11/migrate.go (1)
PendingBallotsDeletionBufferBlocks(19-19)
x/emissions/migrations/v6/migrate.go (1)
x/emissions/types/keys.go (1)
BallotMaturityBlocks(54-54)
🪛 GitHub Check: codecov/patch
x/emissions/migrations/v6/migrate.go
[warning] 34-34: x/emissions/migrations/v6/migrate.go#L34
Added line #L34 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: start-upgrade-test / e2e
- GitHub Check: start-tss-migration-test / e2e
- GitHub Check: start-solana-test / e2e
- GitHub Check: start-e2e-test / e2e
- GitHub Check: start-e2e-performance-test / e2e
🔇 Additional comments (8)
pkg/constant/constant.go (1)
8-8:Details
✅ Verification successful
Block time reduction looks well-coordinated.
The reduction from 6000ms to 4500ms (25% decrease) represents a significant improvement to block production timing. This change appears properly coordinated with the consensus timeout adjustments in
cmd/zetacored/root.go.To ensure this change doesn't introduce timing-related issues, please verify that all timing-dependent components have been updated accordingly:
🏁 Script executed:
#!/bin/bash # Description: Search for hardcoded values that might reference the old 6000ms block time # Expected: Any hardcoded timing values should be reviewed for compatibility echo "Searching for potential hardcoded timing values..." rg -n "6000|6\s*\*\s*time\." --type go echo "" echo "Searching for other block time references..." rg -n "ZetaBlockTime|block.*time|timeout.*commit" --type go -A 2 -B 2Length of output: 90196
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for literal 6000ms block‐time usages in pkg/ and cmd/…" rg -n "6000\s*\*\s*time\.Millisecond" --type go pkg cmd || echo "✔ No literal 6000ms found." echo "" echo "🔍 Searching for literal 6 * time.Second usages in pkg/ and cmd/…" rg -n "6\s*\*\s*time\.Second" --type go pkg cmd || echo "✔ No literal 6 * time.Second found." echo "" echo "🔍 Searching for all ZetaBlockTime references in pkg/ and cmd/…" rg -n "ZetaBlockTime" --type go pkg cmd || echo "✔ No other ZetaBlockTime usages found."Length of output: 901
Block time constant update is consistent
Searches in pkg/ and cmd/ confirm there are no remaining hardcoded6000msor6*time.Secondusages—onlyZetaBlockTime = 4500*time.Millisecondremains. No further changes are necessary.changelog.md (1)
33-33: Documentation properly captures the feature addition.The changelog entry accurately describes the addition of hardcoded block time related parameters, providing appropriate traceability for this significant timing adjustment.
e2e/e2etests/test_bitcoin_donation.go (1)
27-28: Test timing adjustment appropriately maintains stability buffer.The increase from 4 to 6 blocks provides a conservative timing adjustment that accounts for the reduced block time while maintaining test reliability. The total wait time increases slightly from 24 seconds to 27 seconds (6 × 4.5s), which provides additional safety margin for test stability.
cmd/zetacored/root.go (1)
52-52: Consensus timeout reduction properly coordinated with block time changes.The reduction from 5 seconds to 3.5 seconds (30% decrease) aligns well with the block time reduction from 6 seconds to 4.5 seconds (25% decrease). This maintains appropriate consensus timing relationships while improving overall network responsiveness.
x/emissions/keeper/migrator.go (1)
8-8: Migration implementation follows established patterns correctly.The new
Migrate5to6method and import addition are implemented consistently with the existing migration pattern. The code structure is clean and maintainable.Also applies to: 30-33
x/emissions/module.go (1)
25-25: Version upgrade implementation is correct.The consensus version increment and migration registration update are properly aligned and follow the expected pattern for module migrations.
Also applies to: 138-138
x/emissions/migrations/v6/migrate.go (2)
28-28: Clarify the calculation basis for BlockRewardAmount.The hardcoded
BlockRewardAmountvalue uses a very precise decimal string that aligns with the PR author's question about how this value is calculated.Please provide documentation or comments explaining how this specific value was derived, as it's not immediately clear from the codebase:
+ // BlockRewardAmount calculated as: [provide formula/reasoning here] params.BlockRewardAmount = sdkmath.LegacyMustNewDecFromStr("7595486111111111680.000000000000000000")This will improve maintainability and help future developers understand the rationale behind this specific value.
34-34: Error path lacks test coverage.The error case on line 34 is not covered by tests, as indicated by static analysis tools. While this scenario should theoretically not occur, having test coverage would improve confidence in the migration's robustness.
The missing test coverage was already addressed in the previous file review suggestion. This aligns with the static analysis finding.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: x/emissions/migrations/v6/migrate.go#L34
Added line #L34 was not covered by tests
kingpinXD
left a comment
kingpinXD
left a comment
There was a problem hiding this comment.
The upgrade tests are linked to the height , I think we might need to increase this as well in case the first round of tests dont complete on time .
I thionk the tests would still work , even if the upgrade happens before the e2e tests complete but we should increase this number to a time after we expect the e2e runs to be done
is this related to block time decrease? on localnet we dont change actual block time, it is 2s timeout_commit before and after upgrade so should not be affected? |
CharlieMc0
left a comment
CharlieMc0
left a comment
There was a problem hiding this comment.
Need to plan on 4 seconds block times not 4.5s
6 participants
Description
modifying hardcoded values related to block time reduction
it seems some of these can be set via params upgrade, but also updating default values here
Q: how is this value calculated?
How Has This Been Tested?
Summary by CodeRabbit