feat(keys): return of the eth_secp256k1
#1264
Conversation
WalkthroughThe changes primarily focus on integrating Ethereum's secp256k1 cryptographic algorithm into the Cosmos SDK. This includes the addition of new packages and files, updates to existing ones, and modifications to test files to ensure the correct functioning of the new features. The changes also reflect in the configuration files, where the key generation algorithm has been updated to Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- cosmos/crypto/keys/ethsecp256k1/keys.pb.go
- cosmos/go.mod
- cosmos/go.sum
- go.work.sum
Files selected for processing (27)
- cosmos/api/polaris/crypto/ethsecp256k1/v1/keys.pulsar.go (1 hunks)
- cosmos/crypto/codec/codec.go (1 hunks)
- cosmos/crypto/codec/codec_test.go (1 hunks)
- cosmos/crypto/hd/algo.go (1 hunks)
- cosmos/crypto/hd/algo_test.go (1 hunks)
- cosmos/crypto/hd/hd.go (1 hunks)
- cosmos/crypto/hd/hd_test.go (1 hunks)
- cosmos/crypto/keyring/option.go (1 hunks)
- cosmos/crypto/keyring/option_test.go (1 hunks)
- cosmos/crypto/keys/ethsecp256k1/keys.go (1 hunks)
- cosmos/crypto/keys/ethsecp256k1/keys_test.go (1 hunks)
- cosmos/crypto/keys/ethsecp256k1/signature_cgo.go (1 hunks)
- cosmos/crypto/keys/ethsecp256k1/signature_cgo_test.go (1 hunks)
- e2e/hive/clients/polard/entrypoint.sh (1 hunks)
- e2e/precompile/polard/start-node.sh (1 hunks)
- e2e/testapp/app.go (2 hunks)
- e2e/testapp/docker/local/docker-init.sh (1 hunks)
- e2e/testapp/docker/seed/init.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed0-init-step1.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed0-init-step2.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed1-init-step1.sh (1 hunks)
- e2e/testapp/docker/validator/scripts/val0-init-step1.sh (1 hunks)
- e2e/testapp/docker/validator/scripts/val1-init-step1.sh (1 hunks)
- e2e/testapp/entrypoint.sh (1 hunks)
- e2e/testapp/polard/cmd/root.go (2 hunks)
- eth/crypto/imported.go (1 hunks)
- proto/polaris/crypto/ethsecp256k1/v1/keys.proto (1 hunks)
Files not summarized due to errors (1)
- cosmos/api/polaris/crypto/ethsecp256k1/v1/keys.pulsar.go: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- cosmos/api/polaris/crypto/ethsecp256k1/v1/keys.pulsar.go (Error: diff too large)
Files skipped from review due to trivial changes (9)
- cosmos/crypto/codec/codec.go
- cosmos/crypto/codec/codec_test.go
- cosmos/crypto/hd/algo.go
- cosmos/crypto/hd/algo_test.go
- cosmos/crypto/hd/hd_test.go
- cosmos/crypto/keys/ethsecp256k1/signature_cgo_test.go
- e2e/testapp/app.go
- e2e/testapp/docker/seed/init.sh
- proto/polaris/crypto/ethsecp256k1/v1/keys.proto
Additional comments: 40
e2e/testapp/docker/seed/scripts/seed1-init-step1.sh (1)
- 25-29: The default value for
KEYALGOhas been changed to "eth_secp256k1". Ensure that the system supports this new key algorithm and that all dependencies are updated accordingly.e2e/hive/clients/polard/entrypoint.sh (1)
- 24-30: The change in the
KEYALGOvariable from "secp256k1" to "eth_secp256k1" indicates a shift in the cryptographic algorithm used for key generation. Ensure that the new algorithm is compatible with the rest of the system and that all dependencies have been updated accordingly.e2e/testapp/docker/validator/scripts/val1-init-step1.sh (1)
- 22-28: The change in the
KEYALGOvariable from "secp256k1" to "eth_secp256k1" suggests a modification in the cryptographic algorithm used for key generation or verification. Ensure that the new algorithm "eth_secp256k1" is fully integrated and compatible with the rest of the system.- KEYALGO="secp256k1" + KEYALGO="eth_secp256k1"e2e/testapp/docker/seed/scripts/seed0-init-step2.sh (1)
- 25-29: The change from "secp256k1" to "eth_secp256k1" indicates a shift in the cryptographic algorithm used for key generation. Ensure that the rest of the codebase and any dependent systems are compatible with this change.
- KEYALGO="secp256k1" + KEYALGO="eth_secp256k1"e2e/precompile/polard/start-node.sh (1)
- 28-34: The change in the cryptographic algorithm used for key generation and management from "secp256k1" to "eth_secp256k1" is noted. Ensure that this change is compatible with the rest of the system and that all dependencies are updated accordingly.
- KEYALGO="secp256k1" + KEYALGO="eth_secp256k1"e2e/testapp/docker/local/docker-init.sh (1)
- 32-32: The change from "secp256k1" to "eth_secp256k1" indicates a shift in the cryptographic algorithm used for key generation. Ensure that this change is compatible with the rest of the system and that all dependencies are updated accordingly.
- KEYALGO="secp256k1" + KEYALGO="eth_secp256k1"cosmos/crypto/keyring/option.go (3)
1-19: The license header is included correctly. Ensure that the license is compatible with the project's license.
23-27: The imported packages are correct and necessary for the function defined in this file.
29-35: The function
OnlyEthSecp256k1Optionis correctly defined and sets the supported algorithms for keyring options tohd.EthSecp256k1. Ensure that this is the intended behavior and that thehd.EthSecp256k1algorithm is supported everywhere this keyring option is used.e2e/testapp/polard/cmd/root.go (3)
63-70: The new imports
ethcryptocodecandkeyringare introduced to support Ethereum-compatible key generation and management. Ensure these packages are correctly implemented and tested.115-115: The
WithKeyringOptions(keyring.OnlyEthSecp256k1Option())function call sets the keyring to only support theeth_secp256k1option. This restricts the keyring to only generate Ethereum-compatible keys.118-118: The
ethcryptocodec.RegisterInterfaces(interfaceRegistry)function call registers thePubKeyandPrivKeyinterfaces for theeth_secp256k1key type. This is necessary for the keyring to generate and manage Ethereum-compatible keys.e2e/testapp/docker/seed/scripts/seed0-init-step1.sh (1)
- 28-28: The change from "secp256k1" to "eth_secp256k1" indicates a shift in the cryptographic algorithm used for key generation. Ensure that this change is compatible with the rest of the system and that all dependencies are updated accordingly.
- KEYALGO="secp256k1" + KEYALGO="eth_secp256k1"cosmos/crypto/keys/ethsecp256k1/signature_cgo.go (3)
1-19: The license header is included correctly. Ensure that the license is compatible with the project's license.
27-43: The
Signfunction seems to be correctly implemented. It hashes the input if it's not already a hash, converts the private key to ECDSA format, and then signs the hash. However, it's important to note that the function will always hash the input, even if it's already a hash. This could lead to unexpected results if the caller expects the function to sign the input directly without hashing it first. Consider adding a comment to clarify this behavior.45-64: The
VerifySignaturefunction also seems to be correctly implemented. It hashes the message if it's not already a hash, checks the signature length, removes the recovery ID if it's included in the signature, and then verifies the signature. Similar to theSignfunction, it will always hash the message, even if it's already a hash. Consider adding a comment to clarify this behavior.e2e/testapp/entrypoint.sh (1)
- 28-34: The change from "secp256k1" to "eth_secp256k1" is consistent with the pull request summary. Ensure that the new key algorithm "eth_secp256k1" is fully implemented and compatible with the rest of the system.
cosmos/crypto/keys/ethsecp256k1/keys.go (5)
1-19: The license header has been updated. Ensure that the new license is compatible with the project's licensing terms.
23-33: The import paths have been updated. Ensure that these packages are available in the project's dependencies.
35-42: New constants have been defined for private key size, public key size, and key type. These constants seem to be correctly defined.
48-101: The
PrivKeytype has been implemented with methodsBytes(),PubKey(),Equals(),Type(),GenPrivKey(), andToECDSA(). These methods seem to be correctly implemented, but ensure that they are tested thoroughly.106-137: The
PubKeytype has been implemented with methodsAddress(),Bytes(),Type(), andEquals(). These methods seem to be correctly implemented, but ensure that they are tested thoroughly.eth/crypto/imported.go (2)
23-26: The import of
github.com/ethereum/go-ethereum/crypto/secp256k1is new and necessary for the Ethereum-compatible key generation and management.28-49: The variables are assigned functions from the
cryptoandsecp256k1packages. This is a common pattern in Go when you want to expose functions from another package in your package. However, it's important to ensure that these functions are used correctly and that their error returns are properly handled in the code that uses them.e2e/testapp/docker/validator/scripts/val0-init-step1.sh (1)
- 22-28: The change in the
KEYALGOvariable from "secp256k1" to "eth_secp256k1" suggests a modification in the cryptographic algorithm used for key generation or verification. Ensure that the new algorithm "eth_secp256k1" is fully supported and tested in the system. Also, verify that this change doesn't break any existing functionality or dependencies that rely on the old "secp256k1" algorithm.cosmos/crypto/keys/ethsecp256k1/keys_test.go (4)
1-19: The license header is included correctly. Ensure that the license is compatible with the project's license.
23-32: The import statements are correct and necessary for the tests.
34-37: The test suite setup is correct.
39-80: The tests are well-structured and cover the necessary functionality of the
ethsecp256k1package. They test the generation of private keys, the equality and inequality of keys, and the conversion of private keys to Ethereum addresses. The tests also validate the type of the keys.cosmos/crypto/keyring/option_test.go (11)
1-19: The license information is correctly placed at the top of the file. Ensure that the license is compatible with the project's license.
23-41: The import statements are well organized and separated by functionality. Ensure that all imported packages are used in the code.
45-50: The
registerCodecfunction is used to register interfaces. It's a good practice to separate this logic into a function for reusability.52-55: The
TestKeyringfunction is the entry point for running the tests in this file. It uses the Ginkgo BDD testing framework.57-178: The tests are well organized into different contexts and scenarios. Each test has a clear purpose and uses the
BeforeEachandAfterEachhooks to set up and tear down the test environment. This is a good practice as it ensures each test runs in isolation.64-79: The
BeforeEachblock is used to set up the test environment. It creates a temporary directory and a new keyring. Ensure that thekeyring.Newfunction is updated to accept the newOnlyEthSecp256k1Optionoption.81-84: The
AfterEachblock is used to clean up the test environment. It removes the temporary directory created in theBeforeEachblock. This is a good practice as it ensures that each test runs in a clean environment.86-92: This test case checks that the
Keyfunction returns an error when trying to retrieve a non-existent key. This is a good negative test case.94-118: This test case checks that the
NewMnemonicfunction correctly creates a new mnemonic and keyring record. It verifies the mnemonic, key name, key type, and public key type. This is a good positive test case.120-147: This test case checks that the
Derivefunction correctly derives an HD path from a mnemonic. It also checks that the function returns an error when given an incorrect HD path. This is a good positive and negative test case.149-176: This test case checks that the
Generatefunction correctly generates a private key from a derived HD path. It also checks that theBytesToAddressfunction correctly converts the public key address to an Ethereum address. This is a good positive test case.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cosmos/crypto/keyring/option_test.go (1 hunks)
Additional comments: 8
cosmos/crypto/keyring/option_test.go (8)
1-19: The license information is included at the top of the file. Ensure that the use of this license is appropriate for your project and that it is correctly referenced.
23-41: The import statements are correctly formatted and organized. The imported packages are relevant to the tests being performed.
45-50: The
registerCodecfunction is correctly defined and registers the necessary interfaces for the codec.52-55: The
TestKeyringfunction correctly sets up the test suite for thekeyringpackage.57-94: The
BeforeEachandAfterEachfunctions are correctly defined and perform necessary setup and teardown operations for each test. TheKey operationscontext and its test case are correctly defined.96-120: The
NewMnemonic operationcontext and its test cases are correctly defined. TheBeforeEachfunction correctly sets up the necessary variables for the tests.122-150: The
HD path operationscontext and its test cases are correctly defined. TheBeforeEachfunction correctly sets up the necessary variables for the tests.151-179: The
Key generation and retrievalcontext and its test cases are correctly defined. TheBeforeEachfunction correctly sets up the necessary variables for the tests.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1264 +/- ##
==========================================
+ Coverage 48.88% 49.23% +0.35%
==========================================
Files 78 84 +6
Lines 4674 4789 +115
==========================================
+ Hits 2285 2358 +73
- Misses 2227 2257 +30
- Partials 162 174 +12
|
Summary by CodeRabbit
New Features:
hd.EthSecp256k1, providing more control over cryptographic operations.Refactor:
Tests:
Chores: