-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing consensus types #283
Conversation
the chain client was unable to resolve type URL /cosmos.consensus.v1.MsgUpdateParams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[lintersdb] The name "gas" is deprecated. The linter has been renamed to: gosec." WalkthroughThe changes involve integrating the Cosmos SDK's Changes
Poem
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/chain/context.go (1)
157-157
: LGTM! Interface registration added correctly in NewClientContext.The consensus types registration is properly placed and maintains consistency with the NewTxConfig registration pattern.
Consider extracting the common interface registration sequence into a separate function to reduce code duplication between
NewTxConfig
andNewClientContext
. Example:+// registerCommonInterfaces registers all common interfaces used by both TxConfig and ClientContext +func registerCommonInterfaces(interfaceRegistry types.InterfaceRegistry) { + keyscodec.RegisterInterfaces(interfaceRegistry) + std.RegisterInterfaces(interfaceRegistry) + exchange.RegisterInterfaces(interfaceRegistry) + // ... all other common registrations ... + consensustypes.RegisterInterfaces(interfaceRegistry) + feegranttypes.RegisterInterfaces(interfaceRegistry) + wasmtypes.RegisterInterfaces(interfaceRegistry) + icatypes.RegisterInterfaces(interfaceRegistry) +}This would simplify maintenance and ensure consistency between both registration points.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/chain/context.go
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (3)
client/chain/context.go (3)
41-41
: LGTM! Import added correctly.The consensus types import is properly placed with other Cosmos SDK imports and follows the codebase conventions.
109-109
: LGTM! Interface registration added correctly in NewTxConfig.The consensus types registration is properly placed and maintains the logical grouping with other Cosmos SDK type registrations.
41-41
: Verify the changes resolve the type URL issue.The changes appropriately add consensus types registration to both
NewTxConfig
andNewClientContext
, which should resolve the inability to handle/cosmos.consensus.v1.MsgUpdateParams
.Run this script to verify the type URL resolution:
Also applies to: 109-109, 157-157
✅ Verification successful
✅ Consensus message type URL resolution is properly implemented
The verification confirms that:
- The consensus types are properly registered in the interface registry
- The registration is implemented in both required locations
- The implementation follows the standard pattern used for other Cosmos SDK modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the consensus message type URL is properly registered # Search for any existing usage of the consensus message type rg -l "MsgUpdateParams" || echo "No direct usage found" # Verify the interface registration by searching for consensus type registration patterns rg "consensustypes\.RegisterInterfaces" || echo "No registration found"Length of output: 1223
Script:
#!/bin/bash # Check the context of consensustypes.RegisterInterfaces calls rg -B 5 -A 5 "consensustypes\.RegisterInterfaces" client/chain/context.goLength of output: 1140
fix: add consensus types to interface registry
The chain client was unable to resolve type URL
/cosmos.consensus.v1.MsgUpdateParams
Not an urgent fix, can go into
dev
and be part of1.56
once released.Summary by CodeRabbit