fix: comet show-validator command #2131
Conversation
WalkthroughThe changes primarily involve modifications to the Changes
Suggested labels
Suggested reviewers
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2131 +/- ##
==========================================
- Coverage 26.24% 26.23% -0.02%
==========================================
Files 360 360
Lines 16241 16248 +7
Branches 12 12
==========================================
Hits 4263 4263
- Misses 11709 11716 +7
Partials 269 269
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
mod/consensus/pkg/cometbft/cli/commands.go(3 hunks)
🔇 Additional comments (2)
mod/consensus/pkg/cometbft/cli/commands.go (2)
25-26: LGTM: Import additions are appropriate.
The new imports support the custom JSON marshaling functionality required for the public key serialization.
Also applies to: 36-36
167-182: Verify the JSON output format.
Let's verify that the JSON output matches the expected format mentioned in the PR description, particularly the change from cometbft/PubKeyBls12_381 to bls12_381.
✅ Verification successful
Let me gather more information about the JSON serialization format by checking the actual implementation and tests.
Let me check one more thing to verify the actual BLS public key serialization format by looking at tests.
Based on the gathered information, I can now provide the final response:
The JSON output format is consistent with the codebase standards
The implementation in marshalPublicKey correctly uses the BLS key type bls12_381 as defined by CometBLSType constant in mod/primitives/pkg/crypto/bls.go. The JSON structure with type and value fields matches the standard format used across the codebase, as evidenced by similar structures in other parts of the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any hardcoded public key type strings in the codebase
# to ensure consistency with the new format.
echo "Searching for public key type strings..."
rg -i "PubKeyBls12_381|bls12_381" --type go
echo "Searching for other public key type definitions..."
ast-grep --pattern 'Type: $_ // public key type string'
Length of output: 366
Script:
#!/bin/bash
# Check the BLS public key implementation and tests
echo "Checking BLS public key implementation..."
rg -A 10 "func.*Type\(\)" mod/primitives/pkg/crypto/bls.go
echo "Checking test files for JSON serialization..."
rg -l "json.*marshal.*test" --type go | xargs rg -A 10 "Marshal.*bls"
echo "Looking for JSON struct tags in public key related code..."
ast-grep --pattern 'struct {
$$$
Type string `json:"type"`
$$$
}'
Length of output: 1393
Script:
#!/bin/bash
# Check BLS-specific test files and implementation
echo "Checking BLS test files..."
fd -e go test | xargs rg -B 5 -A 10 "json.*Marshal.*bls|bls.*json"
echo "Looking for BLS key type constant usage..."
rg -B 3 -A 3 "CometBLSType" --type go
echo "Checking for any BLS key serialization tests..."
rg -l "TestBLS.*JSON|JSON.*BLS" --type go
Length of output: 2415
|
As a note, if the previous The previous implementation relied on a dependency-injected I've tried to restore the old serialization by replacing the new But the marshaling fails due to the missing PK interface in the registry that I have no clue on how to initialize it… |
|
|
||
| pk := pvm.LoadFilePV( | ||
| cfg.PrivValidatorKeyFile(), | ||
| cfg.PrivValidatorStateFile(), | ||
| ) | ||
| pk, err := privValidator.GetPubKey() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sdkPK, err := cryptocodec.FromCmtPubKeyInterface(pk) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ).Key.PubKey |
There was a problem hiding this comment.
Overall looks good for the fix. I was curious, do we see any benefit in doing this ? I think it wasn't necessary.
|
@berachain/core reasons not to merge this in now? |
The
show-validatorsub-command of thecometone was broken due to the fact that the the codebase no longer seems to provide acosmos-sdk/codec(in the Cosmos context that acts as a service locator).This patch by-pass such codec and provides a custom function for marshalling the public key in JSON.
Please note that the printed
typehas changed fromcometbft/PubKeyBls12_381to a more genericbls12_381, due to the internal representation of such information.Let me know if it's enough or if I should attempt to restore the codec for keeping the same serialization format.
This fixes the issue #2123.
Summary by CodeRabbit
New Features
Bug Fixes