feat: add conversion function from comet type to sdk crypto type #24861
Conversation
Ironbird - launch a networkTo use Ironbird, you can use the following commands:
Custom Load Test ConfigurationYou can provide a custom load test configuration using the `--load-test-config=` flag:Use |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant keys.PubKeyFromTypeAndBytes
participant types.PubKey
Caller->>keys.PubKeyFromTypeAndBytes: Call with pkType, bytes
alt pkType is "ed25519" or "secp256k1"
keys.PubKeyFromTypeAndBytes->>keys.PubKeyFromTypeAndBytes: Validate byte length
alt Valid length
keys.PubKeyFromTypeAndBytes->>types.PubKey: Construct PubKey object
keys.PubKeyFromTypeAndBytes-->>Caller: Return PubKey
else Invalid length
keys.PubKeyFromTypeAndBytes-->>Caller: Return ErrInvalidKeyLen
end
else pkType unsupported
keys.PubKeyFromTypeAndBytes-->>Caller: Return ErrUnsupportedKey
end
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
crypto/keys/convert.go(1 hunks)crypto/keys/convert_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crypto/keys/convert.go (2)
crypto/types/types.go (1)
PubKey(9-17)crypto/keys/ed25519/ed25519.go (2)
KeyType(36-36)PubKeySize(26-26)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: test-system-legacy
- GitHub Check: test-system
- GitHub Check: tests (00)
- GitHub Check: tests (01)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: test-e2e
- GitHub Check: Check up to date mocks
- GitHub Check: test-sim-nondeterminism
- GitHub Check: Analyze
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: golangci-lint
- GitHub Check: build (amd64)
- GitHub Check: dependency-review
- GitHub Check: Gosec
- GitHub Check: Summary
🔇 Additional comments (8)
crypto/keys/convert.go (3)
1-11: LGTM: Clean package structure and imports.The package declaration and imports are well-organized and include all necessary dependencies for the conversion functionality.
13-16: LGTM: Clear function documentation and signature.The function documentation clearly describes the purpose and error conditions. The signature appropriately takes a type string and byte slice, returning the interface type and error.
28-30: LGTM: Secure byte handling.The implementation correctly creates new byte slices and copies the input data, preventing shared references that could lead to security issues.
Also applies to: 41-43
crypto/keys/convert_test.go (5)
1-14: LGTM: Comprehensive test imports and package structure.The test package structure follows Go conventions, and imports include all necessary crypto types for thorough testing.
17-33: LGTM: Thorough ed25519 testing.The test properly validates key reconstruction, equivalence checks, and error handling for invalid key lengths with appropriate error type assertions.
35-48: LGTM: Consistent secp256k1 testing pattern.The secp256k1 testing follows the same comprehensive pattern as ed25519, ensuring consistency in validation.
71-73: LGTM: BLS limitation testing.The test correctly verifies that BLS key generation fails as expected in the current configuration.
75-79: LGTM: Proper unsupported key type testing.The test correctly validates error handling for unsupported key types with proper error type assertion.
| switch pkType { | ||
| case ed25519.KeyType: | ||
| if len(bytes) != ed25519.PubKeySize { | ||
| return nil, encoding.ErrInvalidKeyLen{ | ||
| Key: pkType, | ||
| Got: len(bytes), | ||
| Want: ed25519.PubKeySize, | ||
| } | ||
| } | ||
|
|
||
| pk := make([]byte, ed25519.PubKeySize) | ||
| copy(pk, bytes) | ||
| pubKey = &cosmosed25519.PubKey{Key: pk} | ||
|
|
||
| case secp256k1.KeyType: | ||
| if len(bytes) != secp256k1.PubKeySize { | ||
| return nil, encoding.ErrInvalidKeyLen{ | ||
| Key: pkType, | ||
| Got: len(bytes), | ||
| Want: secp256k1.PubKeySize, | ||
| } | ||
| } | ||
|
|
||
| pk := make([]byte, secp256k1.PubKeySize) | ||
| copy(pk, bytes) | ||
| pubKey = &cosmossecp256k1.PubKey{Key: pk} | ||
| default: | ||
| return nil, encoding.ErrUnsupportedKey{KeyType: pkType} | ||
| } |
There was a problem hiding this comment.
Critical Issue: Missing secp256k1eth support despite test expectations.
The function only handles ed25519 and secp256k1 key types, but the test file (convert_test.go lines 50-69) expects secp256k1eth support when enabled. This will cause test failures when secp256k1eth.Enabled is true.
Add support for secp256k1eth when enabled:
case secp256k1.KeyType:
if len(bytes) != secp256k1.PubKeySize {
return nil, encoding.ErrInvalidKeyLen{
Key: pkType,
Got: len(bytes),
Want: secp256k1.PubKeySize,
}
}
pk := make([]byte, secp256k1.PubKeySize)
copy(pk, bytes)
pubKey = &cosmossecp256k1.PubKey{Key: pk}
+ case secp256k1eth.KeyType:
+ if !secp256k1eth.Enabled {
+ return nil, encoding.ErrUnsupportedKey{KeyType: pkType}
+ }
+ if len(bytes) != secp256k1eth.PubKeySize {
+ return nil, encoding.ErrInvalidKeyLen{
+ Key: pkType,
+ Got: len(bytes),
+ Want: secp256k1eth.PubKeySize,
+ }
+ }
+
+ pk := make([]byte, secp256k1eth.PubKeySize)
+ copy(pk, bytes)
+ pubKey = &cosmossecp256k1.PubKey{Key: pk}
default:
return nil, encoding.ErrUnsupportedKey{KeyType: pkType}You'll also need to add the import:
import (
"github.com/cometbft/cometbft/v2/crypto/ed25519"
"github.com/cometbft/cometbft/v2/crypto/encoding"
"github.com/cometbft/cometbft/v2/crypto/secp256k1"
+ "github.com/cometbft/cometbft/v2/crypto/secp256k1eth"
cosmosed25519 "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
cosmossecp256k1 "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/crypto/types"
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crypto/keys/convert.go lines 18 to 46, the switch statement handles only
ed25519 and secp256k1 key types but lacks support for secp256k1eth, which causes
test failures when secp256k1eth.Enabled is true. To fix this, add a case for
secp256k1eth.KeyType that checks the byte length against
secp256k1eth.PubKeySize, copies the bytes into a new slice, and assigns pubKey
to a new cosmossecp256k1eth.PubKey with the copied key. Also, add the necessary
import for the secp256k1eth package to enable this support.
| // --- secp256k1eth (optional) --- | ||
| if secp256k1eth.Enabled { | ||
| ethPriv := secp256k1eth.GenPrivKey() | ||
| ethPub := ethPriv.PubKey() | ||
|
|
||
| pk, err = keys.PubKeyFromTypeAndBytes(ethPub.Type(), ethPub.Bytes()) | ||
| require.NoError(t, err) | ||
| require.Equal(t, ethPub.Type(), pk.Type()) | ||
| require.Equal(t, ethPub.Bytes(), pk.Bytes()) | ||
| require.Equal(t, ethPub.Address(), pk.Address()) | ||
| require.Equal(t, ethPub.VerifySignature([]byte("msg"), []byte("sig")), pk.VerifySignature([]byte("msg"), []byte("sig"))) | ||
|
|
||
| _, err = keys.PubKeyFromTypeAndBytes(ethPub.Type(), ethPub.Bytes()[:5]) | ||
| require.Error(t, err) | ||
| require.ErrorAs(t, err, &invLen) | ||
| } else { | ||
| // should error if type known but not enabled | ||
| _, err := keys.PubKeyFromTypeAndBytes(secp256k1eth.KeyType, []byte{}) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Test will fail: secp256k1eth not implemented in convert.go.
This test expects secp256k1eth support when enabled, but the PubKeyFromTypeAndBytes function in convert.go doesn't handle this key type, causing test failures.
This test segment will pass only after implementing secp256k1eth support in the conversion function as noted in the previous review comment.
🤖 Prompt for AI Agents
In crypto/keys/convert.go around the implementation of PubKeyFromTypeAndBytes,
the function currently lacks support for the secp256k1eth key type, causing the
test in convert_test.go lines 50 to 69 to fail when secp256k1eth is enabled. To
fix this, add handling logic for the secp256k1eth key type in
PubKeyFromTypeAndBytes so it correctly recognizes and processes keys of this
type, ensuring the test passes when secp256k1eth.Enabled is true.
need to add changelog
Summary by CodeRabbit