feat(blocktime): Use gov-controllednext_block_delay to replace timeout_commit
#2609
Conversation
WalkthroughThe changes in this pull request introduce a new interface Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)protocol/app/msgs/internal_msgs_test.go (1)
The addition of 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 (
|
teddyding
changed the title
next_block_delay to replace timeout_commitnext_block_delay to replace timeout_commit
teddyding
changed the title
next_block_delay to replace timeout_commitnext_block_delay to replace timeout_commit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (13)
protocol/x/blocktime/types/keys.go (1)
23-24: Consider using a consistent naming pattern for state keys.The new
SynchronyParamsKeyuses an abbreviated format"SP:"which differs from the descriptive naming pattern used by other keys (e.g., "DowntimeParams"). Additionally, it includes a colon separator which isn't present in other keys.Consider using a full descriptive name for consistency:
- SynchronyParamsKey = "SP:" + SynchronyParamsKey = "SynchronyParams"If there's a specific reason for using the abbreviated format or the colon separator (e.g., key-space partitioning), please document it in the comment.
protocol/x/blocktime/types/params.go (1)
3-3: Simplify the time package importThe explicit package name "time" is redundant as it matches the default package name.
-import time "time" +import "time"protocol/x/blocktime/keeper/block_delay.go (1)
32-34: Add method documentationConsider adding godoc comments to explain the purpose and return value of this method, especially since it's part of the public API.
+// GetBlockDelay returns the NextBlockDelay duration from the current synchrony parameters. +// If no parameters are set, it returns the default NextBlockDelay value. func (k Keeper) GetBlockDelay(ctx sdk.Context) time.Duration { return k.GetSynchronyParams(ctx).NextBlockDelay }proto/dydxprotocol/blocktime/params.proto (1)
19-24: LGTM! Minor comment improvements suggested.The implementation looks good and aligns well with the PR objectives. However, there's a minor grammatical issue in the comment.
Consider updating the comment for better clarity:
- // CometBFT. It determines the amount of time the CometBFT waits after the If + // CometBFT. It determines the amount of time CometBFT waits after committing a block. + // If the application sends next_block_delay = 0 to the consensus engine, theprotocol/x/blocktime/keeper/msg_server.go (1)
45-48: Add documentation for the new handlerConsider adding a documentation comment explaining the purpose and behavior of this governance-controlled handler, especially its relationship with
next_block_delay.Add a comment like this:
+// UpdateSynchronyParams updates the chain's synchrony parameters through governance. +// These parameters control block timing, specifically the next_block_delay which +// overwrites CometBFT's timeout_commit when non-zero. func (k msgServer) UpdateSynchronyParams( goCtx context.Context, msg *types.MsgUpdateSynchronyParams, ) (*types.MsgUpdateSynchronyParamsResponse, error) {protocol/x/blocktime/keeper/block_delay_test.go (2)
39-49: Consider adding error handling and cleanup.The test execution logic is solid but could be enhanced:
- Add
defercleanup if the test app requires any teardown- Consider testing error cases by validating that invalid parameters are properly rejected
- Add assertions to verify that the chain context is properly initialized (e.g., check block time is set)
Example enhancement:
t.Run(name, func(t *testing.T) { tApp := testapp.NewTestAppBuilder(t).Build() + // Verify initial state + require.NotNil(t, tApp) ctx := tApp.InitChain() + require.NotNil(t, ctx) + require.False(t, ctx.BlockTime().IsZero()) k := tApp.App.BlockTimeKeeper tc.setUp(k, ctx) require.Equal(t, tc.expectedSynchronyParams, k.GetSynchronyParams(ctx)) require.Equal(t, tc.expectedBlockDelay, k.GetBlockDelay(ctx)) + // Add cleanup if needed })
1-49: Consider adding integration tests for CometBFT interaction.While these unit tests cover the basic functionality of synchrony parameters and block delay, consider adding integration tests to verify:
- The interaction with CometBFT's
timeout_commit- The behavior of
next_block_delayinabci.FinalizeBlockResponse- The actual impact on block time in a running network
This would help ensure the complete feature works as intended across the stack.
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.rpc.msg.ts (2)
9-11: Remove unnecessary blank lineThe interface addition looks good, but there's an extra blank line that affects readability.
/** UpdateSynchronyParams updates the SynchronyParams in state. */ - updateSynchronyParams(request: MsgUpdateSynchronyParams): Promise<MsgUpdateSynchronyParamsResponse>;
33-33: Remove extra blank lineThere's an unnecessary blank line before the closing brace.
return promise.then(data => MsgUpdateSynchronyParamsResponse.decode(new _m0.Reader(data))); } - }protocol/x/blocktime/types/tx_test.go (3)
58-103: Consider adding test cases for boundary values.While the test coverage is good for basic scenarios, consider adding test cases for:
- Maximum allowed value for
NextBlockDelay(if any)- Zero value for
NextBlockDelayto explicitly verify if it's allowed
90-90: Fix typo in error constant name.The error constant name has a typo:
ErrNegativeNextBlockDealyshould beErrNegativeNextBlockDelay.
58-103: Consider documenting governance parameter bounds.Since this implements governance-controlled block time, consider adding documentation comments that explain:
- The acceptable range for
next_block_delay- The implications of different values on block time
- Any recommendations for governance proposals
This will help governance participants make informed decisions.
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts (1)
Line range hint
22-132: Consider monitoring and observability for block time changes.Since this feature allows dynamic control of block timing through governance, consider implementing:
- Metrics to track changes in
next_block_delay- Alerts for significant deviations from expected block times
- Logging of governance-initiated block time changes
This will help operators monitor the impact of block time adjustments on network performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
protocol/go.sumis excluded by!**/*.sumprotocol/x/blocktime/types/params.pb.gois excluded by!**/*.pb.goprotocol/x/blocktime/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts(2 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.rpc.msg.ts(2 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts(3 hunks)proto/dydxprotocol/blocktime/params.proto(1 hunks)proto/dydxprotocol/blocktime/tx.proto(2 hunks)protocol/app/app.go(1 hunks)protocol/app/msgs/all_msgs.go(1 hunks)protocol/app/msgs/internal_msgs.go(1 hunks)protocol/go.mod(1 hunks)protocol/lib/ante/internal_msg.go(1 hunks)protocol/x/blocktime/keeper/block_delay.go(1 hunks)protocol/x/blocktime/keeper/block_delay_test.go(1 hunks)protocol/x/blocktime/keeper/msg_server.go(1 hunks)protocol/x/blocktime/types/errors.go(1 hunks)protocol/x/blocktime/types/keys.go(1 hunks)protocol/x/blocktime/types/params.go(2 hunks)protocol/x/blocktime/types/tx.go(1 hunks)protocol/x/blocktime/types/tx_test.go(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts
[error] 53-53: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 59-59: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (26)
protocol/x/blocktime/types/params.go (1)
29-35: Verify upgrade path and add documentation reference
The default value and CometBFT fallback behavior look correct. However:
- Please add a reference to the CometBFT documentation where this fallback behavior is specified.
- Since this is a governance-controlled parameter, we should ensure a smooth upgrade path.
Let's verify the upgrade handling:
protocol/x/blocktime/types/tx.go (2)
24-36: LGTM! Implementation follows Cosmos SDK best practices.
The validation implementation for MsgUpdateSynchronyParams correctly follows Cosmos SDK patterns and maintains consistency with the existing MsgUpdateDowntimeParams validation. The error handling and message formatting are properly implemented.
35-35: Verify the complete validation chain for governance-controlled parameters.
Since this is a governance-controlled feature affecting block time, we should verify that Params.Validate() implements comprehensive validation rules for next_block_delay.
✅ Verification successful
Based on the shell script results, I can see the validation implementation in protocol/x/blocktime/types/params.go:
if s.NextBlockDelay < 0 {
return ErrNegativeNextBlockDealy
}
return nilThe validation only checks for negative values, which is a basic but reasonable validation since:
- The parameter is of type
time.Durationwhich ensures type safety. - Zero is explicitly allowed as per the proto comment: "if the application sends next_block_delay = 0 to the consensus engine, the latter defaults back to using timeout_commit"
- There's no logical upper bound needed since this controls block time delay.
The validation rules for next_block_delay are appropriate for its purpose.
The parameter validation ensures non-negative values, and zero is intentionally allowed to fallback to the default timeout_commit. The type system ensures type safety, and no upper bound is necessary as this is a timing control parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of Params.Validate() for next_block_delay validation
# Search for the Params.Validate implementation
ast-grep --pattern 'func ($p Params) Validate() error {
$$$
}'
# Look for any constants or variables defining validation bounds
rg -A 5 'next_block_delay|NextBlockDelay'
Length of output: 8466
protocol/x/blocktime/keeper/block_delay.go (2)
1-11: LGTM: Package structure and imports are well-organized
The imports are appropriate for the functionality, including the required Cosmos SDK types and time package.
1-34: Verify parameter update safety and usage
Let's verify the usage of these methods and potential race conditions.
✅ Verification successful
Let me gather more information about the parameter update mechanism to ensure safety.
Let me gather more information about the authority checks and parameter validation.
Based on the gathered information, I can now generate the final response:
Parameter updates are properly secured and validated
The synchrony parameter updates are well-protected through multiple layers:
- Authority validation through
HasAuthoritycheck in the message handler - Basic validation through
ValidateBasic()on the message - Single update path through
MsgUpdateSynchronyParams - Proper parameter initialization in genesis
The parameter is only used in GetBlockDelay() which is safely accessed through the keeper interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for concurrent parameter updates and usage patterns
# Find all usages of these methods
echo "Checking method usage patterns..."
rg -A 3 "SetSynchronyParams|GetSynchronyParams|GetBlockDelay" --type go
# Check for potential race conditions in parameter updates
echo "Checking for concurrent parameter updates..."
ast-grep --pattern 'func \($_\) \($_\)(\$_) {
$$$
SetSynchronyParams($$$)
$$$
}'
Length of output: 3011
Script:
#!/bin/bash
# Check for the message handler implementation and authority checks
echo "Checking message handler and authority controls..."
rg -A 10 "MsgUpdateSynchronyParams" --type go
# Check for any other potential parameter update paths
echo "Checking other parameter update paths..."
rg -A 5 "SynchronyParams" --type go --type proto
# Check for genesis handling
echo "Checking genesis handling..."
rg -A 5 "InitGenesis|ExportGenesis" --type go
Length of output: 115654
Script:
#!/bin/bash
# Check for authority validation and parameter validation
echo "Checking authority validation..."
rg -A 5 "HasAuthority|ValidateBasic" --type go
# Check for any synchrony params validation
echo "Checking synchrony params validation..."
rg -A 5 "ValidateSynchronyParams|Validate.*Params" --type go
Length of output: 102832
proto/dydxprotocol/blocktime/params.proto (1)
23-24: Verify duration validation.
Since this duration will be used by CometBFT, we should ensure proper validation of the value.
Let's check how the duration validation is implemented:
✅ Verification successful
Based on the search results, I can now provide a final response. The code shows that there is validation for the next_block_delay parameter in the SynchronyParams.Validate() method, which checks for negative duration values and returns ErrNegativeNextBlockDealy error if the duration is negative. This validation is called during message processing through ValidateBasic() and is sufficient for preventing invalid durations.
Duration validation is properly implemented.
- The
SynchronyParams.Validate()method inparams.govalidates thatNextBlockDelayis not negative - The validation is enforced through the message handler's
ValidateBasic()method - The error
ErrNegativeNextBlockDealyis properly defined and used for validation failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for validation of next_block_delay to prevent negative durations
# or other invalid values that could affect consensus.
# Search for validation in Go files
rg -t go "next_block_delay.*valid"
# Search for specific keeper or message validation methods
ast-grep --pattern 'func $_ ValidateSynchronyParams($_) error {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Let's try a broader search for validation patterns related to blocktime params
# Search for any validation related to blocktime params
rg -t go "blocktime.*valid|ValidateBasic.*blocktime" -A 5
# Look for param validation in the blocktime package
fd -e go . | grep -i "blocktime" | xargs cat
# Search for duration validation patterns
rg -t go "Duration.*validate|validate.*Duration" -A 3
Length of output: 144769
Script:
#!/bin/bash
# Let's search for validation patterns in the types package
# Search for validation related to SynchronyParams
rg -t go "func.*SynchronyParams.*Validate" -A 5
# Look for any validation errors related to duration
rg -t go "ErrNegative.*Duration|Duration.*negative" -A 2
# Search for any validation in msg_server implementation
rg -t go "UpdateSynchronyParams.*ctx.*error" -A 5
Length of output: 5712
protocol/x/blocktime/keeper/block_delay_test.go (1)
14-37: Enhance test coverage with additional test cases.
While the current test cases cover basic functionality, consider adding the following scenarios to improve coverage:
- Edge cases for
NextBlockDelay:- Zero delay
- Maximum allowed delay (if any)
- Invalid delays (negative values)
- State transition cases:
- Updating from non-zero to zero delay
- Updating from one delay to another
Also, consider renaming the test function to better describe its purpose, e.g., TestSynchronyParamsAndBlockDelay.
Let's check if there are any maximum delay constraints defined:
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.rpc.msg.ts (3)
3-3: LGTM: Import statement correctly updated
The new message types for synchrony params are properly imported alongside existing types.
19-19: LGTM: Method properly bound in constructor
The new method is correctly bound to maintain proper this context.
28-32: LGTM: Method implementation follows established patterns
The implementation correctly handles protobuf encoding/decoding and RPC communication.
proto/dydxprotocol/blocktime/tx.proto (4)
16-18: LGTM! RPC method declaration follows best practices.
The UpdateSynchronyParams RPC method is well-defined and follows Cosmos SDK conventions, properly integrating with the existing service structure.
35-43: LGTM! Message type definition is complete and secure.
The MsgUpdateSynchronyParams message type is well-structured with:
- Proper authority authentication using
cosmos.msg.v1.signer - Non-nullable params field to ensure required parameters
- Correct scalar type for the authority address
45-47: LGTM! Response message follows standard patterns.
The empty response message type follows Cosmos SDK conventions for update operations.
Line range hint 7-7: Verify SynchronyParams definition in params.proto
The changes look good, but let's verify that the imported params.proto properly defines the SynchronyParams message type with the next_block_delay field.
✅ Verification successful
SynchronyParams is properly defined with next_block_delay field
The verification confirms that SynchronyParams message type is correctly defined in proto/dydxprotocol/blocktime/params.proto with the next_block_delay field of type google.protobuf.Duration. The field is properly annotated with gogo protobuf options for non-nullable standard duration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SynchronyParams definition in params.proto
# Expected: SynchronyParams message type with next_block_delay field
# Search for SynchronyParams definition
rg -A 10 "message SynchronyParams" proto/dydxprotocol/blocktime/params.proto
Length of output: 506
protocol/x/blocktime/types/tx_test.go (1)
93-101: LGTM! Error handling follows best practices.
The test implementation properly validates errors using require.ErrorIs for type checking and require.NoError for success cases.
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts (2)
90-94: LGTM!
The base creation function correctly initializes the optional nextBlockDelay field as undefined, following the established pattern.
96-132: LGTM!
The encoding, decoding, and partial object creation methods are well-implemented with proper handling of the optional nextBlockDelay field. The implementation follows established protobuf patterns and includes thorough null checks.
protocol/lib/ante/internal_msg.go (1)
80-80: LGTM! The changes follow the established patterns.
The addition of *blocktime.MsgUpdateSynchronyParams is well-placed within the blocktime message group and follows the naming conventions used throughout the codebase.
Let's verify that this message type is properly defined in the blocktime module:
✅ Verification successful
✅ Message type is properly defined and integrated
The message type MsgUpdateSynchronyParams is well-defined in the blocktime module with:
- Complete struct definition with proper fields
- ValidateBasic implementation
- Tests in
tx_test.go - Message server implementation in the keeper
- Generated protobuf code and interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the message type definition exists in the blocktime module
# Expected: Find the message type definition in the blocktime types package
# Search for the message type definition
ast-grep --pattern 'type MsgUpdateSynchronyParams struct {
$$$
}'
# Verify the message is registered in the module
rg -A 5 'RegisterLegacyAminoCodec.*MsgUpdateSynchronyParams'
Length of output: 618
Script:
#!/bin/bash
# Let's verify the message registration in the module using a different approach
# Search for any reference to MsgUpdateSynchronyParams in the module registration
rg "MsgUpdateSynchronyParams" protocol/x/blocktime/
# Check if the message is registered in the module's codec
ast-grep --pattern 'func RegisterInterfaces(registry $_) {
$$$
registry.RegisterImplementations($_,
$$$
&MsgUpdateSynchronyParams{},
$$$
)
$$$
}'
Length of output: 7428
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts (3)
1-1: LGTM: Import changes are appropriate
The addition of SynchronyParams and SynchronyParamsSDKType imports aligns with the existing pattern and supports the new synchrony parameter functionality.
32-59: LGTM: Interface definitions follow consistent patterns
The new interfaces for synchrony parameters (MsgUpdateSynchronyParams, MsgUpdateSynchronyParamsSDKType, and their responses) mirror the structure of existing downtime parameters, maintaining consistency in the codebase. The empty interfaces for responses follow protobuf conventions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 59-59: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
150-236: LGTM: Implementation follows protobuf conventions
The implementation of synchrony parameter methods is thorough and follows established patterns:
- Proper handling of undefined/null cases
- Consistent protobuf encoding/decoding
- Matches existing downtime parameter implementation
Let's verify the integration with the blocktime module:
✅ Verification successful
Let me gather more information to verify the integration completeness.
Based on the verification results, I can now provide the final response:
Implementation and integration are properly structured
The synchrony parameters implementation is correctly integrated within the blocktime module:
- Proto definitions are properly defined with clear RPC endpoints
- Message client implementation includes proper encoding/decoding
- RPC service properly implements the updateSynchronyParams method
- Follows the same pattern as the existing downtime parameters implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of synchrony parameters with the blocktime module
# Check for proper message registration in the module
rg -A 5 "RegisterInterfaces|RegisterImplementations" | grep -i "MsgUpdateSynchronyParams"
# Verify proper proto definitions
fd -e proto | xargs rg "UpdateSynchronyParams"
Length of output: 803
Script:
#!/bin/bash
# Check for message registration in registry files
ast-grep --pattern 'registry.RegisterImplementations($_,$_)'
# Check for handler implementation
rg -A 10 "MsgUpdateSynchronyParams" --type ts
Length of output: 15771
protocol/app/msgs/internal_msgs.go (1)
121-124: LGTM! Message types are properly defined for governance-controlled block time.
The new message types follow the established patterns and are correctly placed within the blocktime module group. The implementation aligns with the PR objectives for implementing governance-controlled block time.
Let's verify the message implementation:
✅ Verification successful
Message types are properly implemented and integrated
The verification confirms that:
- The message struct is correctly defined with proper fields (authority and params)
- The message handler is properly implemented in the keeper with authority checks
- The message server is correctly registered in the module's service registration
- The implementation follows the module's established patterns for governance-controlled parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MsgUpdateSynchronyParams implementation exists and follows the expected pattern
# Check if the message type exists in the blocktime module
ast-grep --pattern 'type MsgUpdateSynchronyParams struct {
$$$
}'
# Check if the message implements expected interfaces
ast-grep --pattern 'func ($MSG MsgUpdateSynchronyParams) $INTERFACE() {
$$$
}'
Length of output: 638
Script:
#!/bin/bash
# Let's verify if the message implements required interfaces for governance messages
# Check for ValidateBasic implementation
ast-grep --pattern 'func (msg MsgUpdateSynchronyParams) ValidateBasic() error {
$$$
}'
# Check for GetSigners implementation
ast-grep --pattern 'func (msg MsgUpdateSynchronyParams) GetSigners() []sdk.AccAddress {
$$$
}'
# Check if there's a proper handler for this message
rg "MsgUpdateSynchronyParams" -A 3 protocol/x/blocktime/keeper/
Length of output: 796
Script:
#!/bin/bash
# Let's verify the complete message implementation and its handler
# Check the full message handler implementation
rg "func \(k msgServer\) UpdateSynchronyParams" -A 15 protocol/x/blocktime/keeper/msg_server.go
# Check if the message is properly registered in the module
rg "RegisterServices" -A 10 protocol/x/blocktime/module.go
Length of output: 1240
protocol/app/msgs/all_msgs.go (1)
183-186: LGTM! Message types are properly structured.
The new blocktime message types follow the consistent naming pattern used throughout the codebase for governance parameter updates.
Let's verify that these message types are properly defined in the protobuf files:
✅ Verification successful
Message types are properly defined in protobuf files
All four message types are correctly defined in proto/dydxprotocol/blocktime/tx.proto:
MsgUpdateDowntimeParamswith proper authority signer optionMsgUpdateDowntimeParamsResponseMsgUpdateSynchronyParamswith proper authority signer optionMsgUpdateSynchronyParamsResponse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the message types are properly defined in proto files
# Expected: Find message definitions in blocktime proto files
# Search for message definitions
rg -A 2 "message (MsgUpdateDowntimeParams|MsgUpdateDowntimeParamsResponse|MsgUpdateSynchronyParams|MsgUpdateSynchronyParamsResponse)"
Length of output: 933
protocol/go.mod (2)
471-471: Verify the backported CometBFT changes
The replacement of CometBFT with dYdX's fork aligns with the PR objectives mentioning backported features.
Let's verify the backported changes:
✅ Verification successful
✅ CometBFT fork changes verified
The changes in the dYdX CometBFT fork (fc0b273d1fbd) are related to the timeout_commit configuration parameter, which aligns with the PR's objective of implementing application-side behavior for block delay. The commit messages and code changes confirm this modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the backported changes in the CometBFT fork
# Check the commit message and changes in the specified commit
gh api repos/dydxprotocol/cometbft/commits/fc0b273d1fbd \
--jq '.commit.message'
# Look for next_block_delay related changes
gh api repos/dydxprotocol/cometbft/compare/v0.38.6...fc0b273d1fbd \
--jq '.files[].patch' | grep -i "next_block_delay"
Length of output: 251
Script:
#!/bin/bash
# Let's try a different approach to verify the changes in the CometBFT fork
# First, let's check the commit directly using the GitHub API with a different endpoint
gh api repos/dydxprotocol/cometbft/git/commits/fc0b273d1fbd \
--jq '.message'
# Let's also check for timeout_commit related changes since that's mentioned in the commit message
gh api repos/dydxprotocol/cometbft/commits/fc0b273d1fbd \
--jq '.files[].patch' | grep -i "timeout_commit"
# Let's also check the PR that introduced these changes
gh api repos/dydxprotocol/cometbft/commits/fc0b273d1fbd \
--jq '.parents[0].sha' | xargs -I {} gh api repos/dydxprotocol/cometbft/commits/{} \
--jq '.commit.message'
Length of output: 776
473-473: Verify Cosmos SDK version compatibility
The Cosmos SDK fork version appears to be recent and specific (using a git commit hash), which is good for reproducibility.
Let's verify the version compatibility:
✅ Verification successful
Cosmos SDK fork version is compatible and properly referenced
The verification shows that:
- The fork is properly referenced with a specific commit hash (7b365908c58e)
- The commit message "Next block delay" suggests a minor change
- The go.mod file shows proper dependency management with necessary replacements
- The repository is using a fork of Cosmos SDK v0.50.6 which is a stable version
- All related dependencies (CometBFT, IAVL, store) are also properly forked and versioned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Cosmos SDK version compatibility
# Check the commit message for the specified version
gh api repos/dydxprotocol/cosmos-sdk/commits/7b365908c58e \
--jq '.commit.message'
# Look for any breaking changes or compatibility notes
gh api repos/dydxprotocol/cosmos-sdk/compare/v0.50.6...7b365908c58e \
--jq '.files[].patch' | grep -i "BREAKING"
Length of output: 233
Script:
#!/bin/bash
# Let's try a different approach to verify the Cosmos SDK version compatibility
# Check the go.mod file for any additional context about the dependency
cat go.mod
# Look for any migration guides or upgrade notes in the repository
fd -t f "UPGRADING|MIGRATION|CHANGELOG" -i
# Check for any cosmos-sdk related configuration or version files
fd -t f "cosmos" -i
Length of output: 481
Script:
#!/bin/bash
# Let's check the correct path for go.mod and analyze its contents
fd -t f "go.mod"
# Once we find it, let's look at its contents
cat protocol/go.mod
# Let's also check for any recent changes or documentation related to the Cosmos SDK upgrade
rg -i "cosmos.* sdk" protocol/README.md protocol/CHANGELOG.md 2>/dev/null || true
Length of output: 25339
protocol/app/app.go (1)
1572-1573: LGTM! Block delay getter is properly integrated.
The integration of BlockTimeKeeper.GetBlockDelay as the block delay getter is correctly placed after module initialization and before state loading. This change aligns with implementing the governance-controlled block time feature.
Let's verify the BlockTimeKeeper initialization and usage:
✅ Verification successful
Block delay integration is properly implemented and initialized
The verification confirms:
BlockTimeKeeperis correctly initialized usingblocktimemodulekeeper.NewKeeperGetBlockDelayis properly defined in the blocktime keeper- The integration point in
app.gocorrectly connects the keeper's method to the app's block delay getter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BlockTimeKeeper initialization and usage
# Test: Check for BlockTimeKeeper initialization and GetBlockDelay method
rg -A 5 "BlockTimeKeeper\s*=\s*\*?blocktimemodulekeeper\.NewKeeper"
# Test: Check for other usages of GetBlockDelay
rg "GetBlockDelay"
Length of output: 866
protocol/x/blocktime/types/errors.go
Outdated
| ErrNegativeNextBlockDealy = errorsmod.Register( | ||
| ModuleName, | ||
| 403, | ||
| "next_block_delay must be non-negative", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo and improve error message formatting
- There's a typo in the error variable name:
ErrNegativeNextBlockDealyshould beErrNegativeNextBlockDelay. - Consider using camelCase in the error message for consistency with Go conventions:
nextBlockDelayinstead ofnext_block_delay.
- ErrNegativeNextBlockDealy = errorsmod.Register(
+ ErrNegativeNextBlockDelay = errorsmod.Register(
ModuleName,
403,
- "next_block_delay must be non-negative",
+ "nextBlockDelay must be non-negative",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ErrNegativeNextBlockDealy = errorsmod.Register( | |
| ModuleName, | |
| 403, | |
| "next_block_delay must be non-negative", | |
| ) | |
| ErrNegativeNextBlockDelay = errorsmod.Register( | |
| ModuleName, | |
| 403, | |
| "nextBlockDelay must be non-negative", | |
| ) |
protocol/x/blocktime/types/params.go
Outdated
| func (s SynchronyParams) Validate() error { | ||
| if s.NextBlockDelay < 0 { | ||
| return ErrNegativeNextBlockDealy | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in error variable name and consider adding upper bound validation
- The error variable has a typo:
ErrNegativeNextBlockDealyshould beErrNegativeNextBlockDelay - Since no existing upper bound constants were found in the blocktime module and this parameter affects consensus timing, consider adding an upper bound validation to prevent potential consensus issues with extremely large delays.
Example fix:
func (s SynchronyParams) Validate() error {
if s.NextBlockDelay < 0 {
return ErrNegativeNextBlockDelay // Fix typo in error name
}
// Consider adding upper bound check
return nil
}🔗 Analysis chain
Fix typo in error variable name and consider adding upper bound validation
- There appears to be a typo in the error variable name:
ErrNegativeNextBlockDealyshould beErrNegativeNextBlockDelay. - Consider adding an upper bound validation for
NextBlockDelayto prevent potential consensus issues with extremely large delays.
Let's verify the error variable definition and check for any existing upper bound constraints:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error variable definition and related constants
rg "ErrNegativeNextBlock(Dealy|Delay)" --type go
# Look for any existing time-related constants or bounds
rg "MaxBlockDelay|MaxTimeout|BlockTimeout" --type go
Length of output: 350
Script:
#!/bin/bash
# Let's check the error definition and its usage context
rg -A 2 "ErrNegativeNextBlockDealy" protocol/x/blocktime/types/errors.go
# Let's also check for any time-related parameters or configurations
ast-grep --pattern 'const $_= time.$_'
rg "time\." --type go -A 2
Length of output: 101264
Script:
#!/bin/bash
# Let's check for any existing max/upper bound constants in the blocktime module
rg "Max.*Block.*" protocol/x/blocktime/types/
rg "const.*Block.*" protocol/x/blocktime/types/
# Let's also check the error definition and its usage context
cat protocol/x/blocktime/types/errors.go
Length of output: 649
| var params types.SynchronyParams | ||
| k.cdc.MustUnmarshal(bytes, ¶ms) | ||
| fmt.Printf("Returning un-nil synchrony params: %v\n", params) |
There was a problem hiding this comment.
Add error handling for parameter unmarshaling
Using MustUnmarshal can cause panics if the stored data is corrupted. Consider using regular Unmarshal with error handling.
- k.cdc.MustUnmarshal(bytes, ¶ms)
+ if err := k.cdc.Unmarshal(bytes, ¶ms); err != nil {
+ k.Logger(ctx).Error("failed to unmarshal synchrony params", "error", err)
+ return types.DefaultSynchronyParams()
+ }Committable suggestion skipped: line range outside the PR's diff.
| func (k Keeper) GetSynchronyParams(ctx sdk.Context) types.SynchronyParams { | ||
| store := ctx.KVStore(k.storeKey) | ||
| bytes := store.Get([]byte(types.SynchronyParamsKey)) | ||
|
|
||
| if bytes == nil { | ||
| fmt.Printf("!! Returning default synchrony params\n") | ||
| return types.DefaultSynchronyParams() | ||
| } | ||
|
|
||
| var params types.SynchronyParams | ||
| k.cdc.MustUnmarshal(bytes, ¶ms) | ||
| fmt.Printf("Returning un-nil synchrony params: %v\n", params) | ||
| return params | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fmt.Printf with proper logging
The debug print statements should use the Cosmos SDK logger instead of fmt.Printf for proper log level control and structured logging.
- fmt.Printf("!! Returning default synchrony params\n")
+ k.Logger(ctx).Debug("returning default synchrony params")- fmt.Printf("Returning un-nil synchrony params: %v\n", params)
+ k.Logger(ctx).Debug("retrieved synchrony params", "params", params)Committable suggestion skipped: line range outside the PR's diff.
| func (k Keeper) SetSynchronyParams(ctx sdk.Context, params types.SynchronyParams) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| store.Set([]byte(types.SynchronyParamsKey), k.cdc.MustMarshal(¶ms)) | ||
| } |
There was a problem hiding this comment.
Add validation and error handling for parameter setting
The method should validate parameters and handle potential marshaling errors.
func (k Keeper) SetSynchronyParams(ctx sdk.Context, params types.SynchronyParams) {
+ if err := params.Validate(); err != nil {
+ panic(fmt.Sprintf("invalid synchrony params: %s", err))
+ }
store := ctx.KVStore(k.storeKey)
- store.Set([]byte(types.SynchronyParamsKey), k.cdc.MustMarshal(¶ms))
+ bz, err := k.cdc.Marshal(¶ms)
+ if err != nil {
+ panic(fmt.Sprintf("failed to marshal synchrony params: %s", err))
+ }
+ store.Set([]byte(types.SynchronyParamsKey), bz)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k Keeper) SetSynchronyParams(ctx sdk.Context, params types.SynchronyParams) { | |
| store := ctx.KVStore(k.storeKey) | |
| store.Set([]byte(types.SynchronyParamsKey), k.cdc.MustMarshal(¶ms)) | |
| } | |
| func (k Keeper) SetSynchronyParams(ctx sdk.Context, params types.SynchronyParams) { | |
| if err := params.Validate(); err != nil { | |
| panic(fmt.Sprintf("invalid synchrony params: %s", err)) | |
| } | |
| store := ctx.KVStore(k.storeKey) | |
| bz, err := k.cdc.Marshal(¶ms) | |
| if err != nil { | |
| panic(fmt.Sprintf("failed to marshal synchrony params: %s", err)) | |
| } | |
| store.Set([]byte(types.SynchronyParamsKey), bz) | |
| } |
| func (k msgServer) UpdateSynchronyParams( | ||
| goCtx context.Context, | ||
| msg *types.MsgUpdateSynchronyParams, | ||
| ) (*types.MsgUpdateSynchronyParamsResponse, error) { | ||
| if !k.HasAuthority(msg.Authority) { | ||
| return nil, errorsmod.Wrapf( | ||
| govtypes.ErrInvalidSigner, | ||
| "invalid authority %s", | ||
| msg.Authority, | ||
| ) | ||
| } | ||
|
|
||
| ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName) | ||
| k.SetSynchronyParams(ctx, msg.Params) | ||
|
|
||
| return &types.MsgUpdateSynchronyParamsResponse{}, nil | ||
| } |
There was a problem hiding this comment.
Add error handling for SetSynchronyParams
The UpdateSynchronyParams handler ignores potential errors from SetSynchronyParams, unlike its counterpart UpdateDowntimeParams. This could mask important validation errors or state update failures.
Apply this diff to add proper error handling:
func (k msgServer) UpdateSynchronyParams(
goCtx context.Context,
msg *types.MsgUpdateSynchronyParams,
) (*types.MsgUpdateSynchronyParamsResponse, error) {
if !k.HasAuthority(msg.Authority) {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidSigner,
"invalid authority %s",
msg.Authority,
)
}
ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName)
- k.SetSynchronyParams(ctx, msg.Params)
+ if err := k.SetSynchronyParams(ctx, msg.Params); err != nil {
+ return nil, err
+ }
return &types.MsgUpdateSynchronyParamsResponse{}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k msgServer) UpdateSynchronyParams( | |
| goCtx context.Context, | |
| msg *types.MsgUpdateSynchronyParams, | |
| ) (*types.MsgUpdateSynchronyParamsResponse, error) { | |
| if !k.HasAuthority(msg.Authority) { | |
| return nil, errorsmod.Wrapf( | |
| govtypes.ErrInvalidSigner, | |
| "invalid authority %s", | |
| msg.Authority, | |
| ) | |
| } | |
| ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName) | |
| k.SetSynchronyParams(ctx, msg.Params) | |
| return &types.MsgUpdateSynchronyParamsResponse{}, nil | |
| } | |
| func (k msgServer) UpdateSynchronyParams( | |
| goCtx context.Context, | |
| msg *types.MsgUpdateSynchronyParams, | |
| ) (*types.MsgUpdateSynchronyParamsResponse, error) { | |
| if !k.HasAuthority(msg.Authority) { | |
| return nil, errorsmod.Wrapf( | |
| govtypes.ErrInvalidSigner, | |
| "invalid authority %s", | |
| msg.Authority, | |
| ) | |
| } | |
| ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName) | |
| if err := k.SetSynchronyParams(ctx, msg.Params); err != nil { | |
| return nil, err | |
| } | |
| return &types.MsgUpdateSynchronyParamsResponse{}, nil | |
| } |
| ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName) | ||
| k.SetSynchronyParams(ctx, msg.Params) |
There was a problem hiding this comment.
💡 Codebase verification
Parameter validation is present but minimal
The SynchronyParams.Validate() method only checks that NextBlockDelay is not negative. While this prevents invalid negative values, it may not be sufficient to prevent potential consensus issues. Consider adding additional validation:
- Maximum value bounds to prevent excessively long delays
- Alignment with network timing constraints
- Validation against CometBFT's timeout configuration
🔗 Analysis chain
Verify parameter validation
Ensure that SetSynchronyParams performs proper validation of the next_block_delay parameter to prevent potential consensus issues.
Let's verify the validation logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for parameter validation in SetSynchronyParams
# Expected: Find validation logic for next_block_delay
# Look for the keeper method implementation
ast-grep --pattern 'func $_($_ Keeper) SetSynchronyParams($_, $params $_) $_ {
$$$
}'
# Look for any validation functions for SynchronyParams
rg -A 5 'func.*Validate.*SynchronyParams'
Length of output: 133
Script:
#!/bin/bash
# Let's try a different approach to find the validation logic
# Search for the SetSynchronyParams implementation
rg -A 10 "SetSynchronyParams"
# Search for SynchronyParams struct and its validation
rg -A 10 "type SynchronyParams"
# Look for any Validate methods in the types package
fd "types" --type f --exec rg -l "func.*Validate"
Length of output: 2915
Script:
#!/bin/bash
# Let's check for validation in the message handling and genesis validation
# Check for validation in MsgUpdateSynchronyParams
rg -A 10 "MsgUpdateSynchronyParams.*Validate"
# Look for genesis validation
rg -A 10 "ValidateGenesis"
# Check for any parameter validation in the types package
rg -A 10 "func.*Validate.*Params" protocol/x/blocktime/types/
Length of output: 53202
Script:
#!/bin/bash
# Let's check for validation in the message and genesis types
# Look for SynchronyParams validation in genesis
rg -A 10 "func.*GenesisState.*Validate" protocol/x/blocktime/types/
# Look for any validation in the params.go file
rg -A 10 "func.*Validate" protocol/x/blocktime/types/params.go
# Check for any validation in the tx.go file beyond ValidateBasic
rg -A 10 "func.*Validate" protocol/x/blocktime/types/tx.go
Length of output: 2569
| /** SynchronyParams defines the parameters for block synchrony. */ | ||
|
|
||
| export interface SynchronyParams { | ||
| /** | ||
| * next_block_delay replaces the locally configured timeout_commit in | ||
| * CometBFT. It determines the amount of time the CometBFT waits after the If | ||
| * the application sends next_block_delay = 0 to the consensus engine, the | ||
| * latter defaults back to using timeout_commit. | ||
| */ | ||
| nextBlockDelay?: Duration; | ||
| } | ||
| /** SynchronyParams defines the parameters for block synchrony. */ | ||
|
|
||
| export interface SynchronyParamsSDKType { | ||
| /** | ||
| * next_block_delay replaces the locally configured timeout_commit in | ||
| * CometBFT. It determines the amount of time the CometBFT waits after the If | ||
| * the application sends next_block_delay = 0 to the consensus engine, the | ||
| * latter defaults back to using timeout_commit. | ||
| */ | ||
| next_block_delay?: DurationSDKType; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Fix documentation typo and add validation constraint information.
The documentation should be updated to:
- Fix the incomplete sentence in the comment
- Add the validation constraint that
next_block_delaymust be non-negative, as found in the Go implementation (protocol/x/blocktime/types/errors.go)
The updated documentation should look like:
/**
* next_block_delay replaces the locally configured timeout_commit in
* CometBFT. It determines the amount of time CometBFT waits after committing a block.
* If the application sends next_block_delay = 0 to the consensus engine, the
* latter defaults back to using timeout_commit.
* Note: The value must be non-negative.
*/🔗 Analysis chain
Fix documentation typo and consider adding validation constraints.
There's an incomplete sentence in the documentation: "It determines the amount of time the CometBFT waits after the". Additionally, consider documenting any validation constraints for the next_block_delay value (e.g., minimum/maximum allowed values).
Let's check if there are any validation constraints defined in the proto files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation rules or constraints related to next_block_delay
rg -A 5 "next_block_delay" | grep -i "valid"
Length of output: 42
Script:
#!/bin/bash
# Let's first check the proto file definitions for next_block_delay
fd -e proto | xargs rg -l "next_block_delay"
Length of output: 86
Script:
#!/bin/bash
# Search for any validation or constraints in the Go implementation
rg -A 10 "next_block_delay" --type go
Length of output: 1600
Script:
#!/bin/bash
# Search for any validation or constraints in the TypeScript implementation
rg -A 10 "next_block_delay" --type ts
Length of output: 2954
Script:
#!/bin/bash
# Look for any parameter validation patterns in the codebase
ast-grep --pattern 'func ValidateBasic() error { $$$ }'
Length of output: 57
| bytes := store.Get([]byte(types.SynchronyParamsKey)) | ||
|
|
||
| if bytes == nil { | ||
| fmt.Printf("!! Returning default synchrony params\n") |
There was a problem hiding this comment.
nit: Use a logger rather than using fmt.Printf?
There was a problem hiding this comment.
Lol debug statement
| @@ -19,4 +19,7 @@ const ( | |||
|
|
|||
| // PreviousBlockInfoKey defines the key for PreviousBlockInfo | |||
| PreviousBlockInfoKey = "PreviousBlockInfo" | |||
|
|
|||
| // SynchronyParamsKey defines the key for the SynchronyParams | |||
| SynchronyParamsKey = "SP:" | |||
There was a problem hiding this comment.
nit: Any reason not to just use the full string SynchronyParams?
There was a problem hiding this comment.
Conventionally we prefer more succinct keys to more efficiently compute them, so it's the other keys are breaking conventions a bit.
teddyding
force-pushed
the
td/next-block-delay
branch
from
5cfa661 to
1efbf60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts (1)
133-133: Consider using nullish coalescing for cleaner null checks.The current null check can be simplified using the nullish coalescing operator.
- message.nextBlockDelay = object.nextBlockDelay !== undefined && object.nextBlockDelay !== null ? Duration.fromPartial(object.nextBlockDelay) : undefined; + message.nextBlockDelay = object.nextBlockDelay ?? undefined;indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts (1)
32-59: LGTM: Well-structured interface definitions for synchrony parameters.The new interfaces follow the established pattern and are well-documented. The empty response interfaces follow protobuf conventions for empty messages.
Note: While the static analyzer flags empty interfaces, this is a common pattern in protobuf-generated code and is intentional here. No changes needed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 59-59: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
protocol/go.sumis excluded by!**/*.sumprotocol/x/blocktime/types/params.pb.gois excluded by!**/*.pb.goprotocol/x/blocktime/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts(2 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.rpc.msg.ts(2 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts(3 hunks)proto/dydxprotocol/blocktime/params.proto(1 hunks)proto/dydxprotocol/blocktime/tx.proto(2 hunks)protocol/app/app.go(1 hunks)protocol/app/msgs/all_msgs.go(1 hunks)protocol/app/msgs/internal_msgs.go(1 hunks)protocol/go.mod(1 hunks)protocol/lib/ante/internal_msg.go(1 hunks)protocol/x/blocktime/keeper/block_delay.go(1 hunks)protocol/x/blocktime/keeper/block_delay_test.go(1 hunks)protocol/x/blocktime/keeper/msg_server.go(1 hunks)protocol/x/blocktime/types/errors.go(1 hunks)protocol/x/blocktime/types/keys.go(1 hunks)protocol/x/blocktime/types/params.go(2 hunks)protocol/x/blocktime/types/tx.go(1 hunks)protocol/x/blocktime/types/tx_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.rpc.msg.ts
- proto/dydxprotocol/blocktime/params.proto
- protocol/app/app.go
- protocol/app/msgs/all_msgs.go
- protocol/app/msgs/internal_msgs.go
- protocol/go.mod
- protocol/lib/ante/internal_msg.go
- protocol/x/blocktime/keeper/block_delay.go
- protocol/x/blocktime/keeper/block_delay_test.go
- protocol/x/blocktime/keeper/msg_server.go
- protocol/x/blocktime/types/errors.go
- protocol/x/blocktime/types/keys.go
- protocol/x/blocktime/types/params.go
- protocol/x/blocktime/types/tx.go
- protocol/x/blocktime/types/tx_test.go
🧰 Additional context used
🪛 Biome (1.9.4)
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts
[error] 53-53: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 59-59: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (10)
proto/dydxprotocol/blocktime/tx.proto (4)
16-18: LGTM! RPC method declaration is well-structured.
The new UpdateSynchronyParams RPC method follows consistent naming and structure with the existing UpdateDowntimeParams method, and aligns with the PR objective of implementing governance-controlled block time.
35-43: LGTM! Message definition follows Cosmos SDK conventions.
The MsgUpdateSynchronyParams message is well-structured with:
- Proper authority field configuration using cosmos address scalar
- Non-nullable params field
- Clear documentation
- Correct protobuf options
45-47: LGTM! Response message follows standard pattern.
The empty response message type follows the Cosmos SDK convention for update operations and maintains consistency with the existing MsgUpdateDowntimeParamsResponse.
Line range hint 8-8: Verify SynchronyParams definition in params.proto
Let's ensure the imported params.proto file contains the required SynchronyParams message type.
✅ Verification successful
Import dependency verified successfully
The params.proto file contains the required SynchronyParams message type with a properly defined next_block_delay field using google.protobuf.Duration. The import statement in tx.proto is valid and correctly references the dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SynchronyParams message type exists in params.proto
# Expected: Should find the SynchronyParams message definition
rg -A 5 "message SynchronyParams" proto/dydxprotocol/blocktime/params.proto
Length of output: 439
Script:
#!/bin/bash
# Let's get the complete definition of SynchronyParams message
rg -A 15 "message SynchronyParams" proto/dydxprotocol/blocktime/params.proto
Length of output: 613
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/params.ts (3)
25-33: Documentation needs improvement.
The documentation still needs to be updated to:
- Fix the incomplete sentence that ends with "after the"
- Add the validation constraint that
next_block_delaymust be non-negative
37-47: LGTM!
The SDK type correctly mirrors the main interface with appropriate naming conventions.
24-34: Verify alignment with CometBFT implementation.
Since this interface is used to communicate with CometBFT, let's verify that the protobuf definitions align with CometBFT's implementation.
✅ Verification successful
Let me generate additional verification scripts to check the protobuf definitions directly.
Based on the verification results, I can now provide the final response:
Protobuf definitions correctly align with CometBFT implementation
The implementation is correctly defined and matches the expected integration with CometBFT:
- The protobuf definition in
proto/dydxprotocol/blocktime/params.protoproperly definesnext_block_delayas agoogle.protobuf.Durationwith the correct stdduration conversion. - The generated code and tests show proper validation (preventing negative values) and default initialization.
- The implementation includes appropriate error handling and parameter management through the blocktime module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the protobuf definitions match CometBFT's implementation
# Search for CometBFT protobuf definitions
rg -A 5 "next_block_delay" --type proto
# Search for related CometBFT structs/interfaces
rg -A 5 "NextBlockDelay" --type go
Length of output: 5636
Script:
#!/bin/bash
# Search for the original proto file definitions
fd -e proto | xargs rg -l "next_block_delay"
# Check the content of any proto files found
fd "params.proto" | xargs cat
Length of output: 13595
indexer/packages/v4-protos/src/codegen/dydxprotocol/blocktime/tx.ts (3)
1-1: LGTM: Import changes align with new synchrony parameters.
The addition of SynchronyParams and SynchronyParamsSDKType imports supports the new block time control feature.
150-236: LGTM: Robust implementation of synchrony parameter message handling.
The implementation:
- Follows protobuf best practices
- Includes proper null/undefined checks
- Handles encoding/decoding correctly
- Maintains consistency with existing patterns
34-39: Verify governance control implementation.
The message structure includes authority field which is crucial for governance control. Let's verify the governance integration:
✅ Verification successful
Governance control implementation verified successfully
The implementation properly enforces governance control through:
- Required
authorityfield in the message structure - Authority validation in the message server implementation using
HasAuthority - Integration with governance error types (
govtypes.ErrInvalidSigner) - Proper parameter updates only after authority validation
The code follows the standard pattern for governance-controlled parameter updates in the protocol.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify governance integration for synchrony params
# Check for governance proposal handling
rg -A 5 "MsgUpdateSynchronyParams" --type proto
# Verify authority validation
rg -A 5 "ValidateAuthority.*blocktime" --type go
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search without file type restrictions
# Check for message definition and usage
rg -A 5 "MsgUpdateSynchronyParams"
# Look for authority validation patterns
rg -A 5 "authority.*blocktime"
# Search for proto definitions
fd "\.proto$" | xargs cat
Length of output: 134828
Script:
#!/bin/bash
# Let's verify the governance control implementation by checking the proto definition and validation
# Check proto definition for governance control
fd "blocktime.*tx.proto$" | xargs cat
# Check for authority validation in Go code
rg -A 5 "HasAuthority.*blocktime" --type go
Length of output: 85
Script:
#!/bin/bash
# Let's try a different approach to verify the governance control
# Check for authority validation in keeper
rg -A 5 "msg\.Authority" protocol/x/blocktime/keeper/
# Check for authority validation in any Go files
rg -A 5 "authority.*blocktime" protocol/x/blocktime/
# Look for msg server implementation
rg -A 10 "UpdateSynchronyParams" protocol/x/blocktime/keeper/msg_server.go
Length of output: 2000
Changelist
(This PR uses temporary feature branches of
dydxprotocol/cosmos-sdkanddydxprotocol/cometbft, to be merged first after approval).Gov-controlled blocktime:
next_block_delayinx/blocktimemodule, sends tocometbftviaabci.FinalizeBlockResponse.timeout_commitwhichnext_block_delayif it's non-zero.Note that
next_block_delayis a released feature oncosmos/cometbft(backported to dydxdprotocol/cometbft), and we are just implementing the app-side behavior ofnext_block_delayTest Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Release Notes
New Features
SynchronyParamsfor managing block synchrony with an optionalnextBlockDelay.updateSynchronyParamsmethod to update synchrony parameters via RPC.SynchronyParamsandMsgUpdateSynchronyParams.Bug Fixes
nextBlockDelay.Tests
These enhancements improve the protocol's capability to manage block timing and synchrony effectively.