feat: add GetStatus to tmclient #247
Conversation
WalkthroughThe pull request introduces significant enhancements to the Cosmos SDK keyring management, including the addition of a new keyring helper, error handling improvements, and updates to the testing framework. The workflow for running tests has been modified to include GPG key setup and password management. Additionally, various files have been created or updated to support the new functionalities, including error definitions and configuration options for the keyring. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (17)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 LanguageTool
🪛 Gitleaks
🔇 Additional comments (19)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (30)
.pre-commit-config.yaml (2)
Line range hint
18-18: Consider using a specific version for pre-commit-golang.Currently, the
pre-commit-golangrepository is set to use themasterbranch. This can lead to unexpected changes in behavior if the repository is updated. It's generally a good practice to pin to a specific version or commit hash for better reproducibility and stability.Consider changing the
revfield to a specific version or commit hash. For example:- repo: https://github.com/dnephin/pre-commit-golang rev: v0.5.1 # Replace with the latest stable version hooks: - id: go-fmt - id: go-imports - id: golangci-lint args: [--timeout=15m]
Line range hint
13-14: Remove duplicate end-of-file-fixer hook.The
end-of-file-fixerhook is listed twice under thepre-commit-hooksrepository. This is likely an oversight and may cause the hook to run unnecessarily twice.Remove one of the duplicate entries:
hooks: - id: trailing-whitespace - id: end-of-file-fixer - - id: end-of-file-fixer - id: check-yamlclient/keyring/testdata/keyring-file/test2.info (1)
1-1: Add a newline at the end of the fileThe file currently lacks a newline character at the end. While this doesn't affect the JWT itself, it's a good practice to end text files with a newline. This can prevent issues with certain text processing tools and improves compatibility across different systems.
Consider adding a newline character at the end of the file.
.github/workflows/run-tests.yml (2)
18-19: Removesudofrom the apt-get commands.GitHub Actions runners already have root privileges, so using
sudois unnecessary and can be removed.Apply this change:
- run: sudo apt-get update && sudo apt-get install -y pass + run: apt-get update && apt-get install -y pass
23-24: Consider using a more appropriate name for the keyring.The current setup uses
keyring_testas the name for both the GPG key and the password store. While this is consistent, it suggests a test environment, which may not be appropriate for a production workflow.Consider using a more descriptive and production-appropriate name, such as
github_actions_keyringorci_cd_keyring. For example:- run: pass init keyring_test + run: pass init github_actions_keyringRemember to update the
Name-Commentin the GPG key generation step as well to maintain consistency.client/keyring/errors.go (2)
5-20: LGTM with suggestions: Error variables are well-structured, but some messages could be more specific.The error variables are correctly defined using
errors.New()and are appropriately exported. The messages are generally clear and follow a consistent style. However, some error messages could be more specific or provide more context:
- For
ErrCosmosKeyringCreationFailedandErrCosmosKeyringImportFailed, consider adding more context about what might cause these failures.ErrFilepathIncorrectcould be more specific, e.g., "invalid filepath format" or "filepath not found".ErrHexFormatErrorcould specify what aspect of the hex format is incorrect.ErrIncompatibleOptionsProvidedcould benefit from mentioning which options are incompatible.Consider revising these error messages to provide more specific information, which could aid in debugging and error handling.
1-20: Suggestion: Add comments and documentation to improve code readability.While the file structure is clean and focused, it would benefit from additional documentation:
- Consider adding a package-level comment at the top of the file explaining the purpose of these error definitions.
- Add comments for each exported error variable, describing when and why these errors might occur. This is especially important for exported variables in Go.
Example:
// Package keyring provides error definitions for keyring operations. // ErrCosmosKeyringCreationFailed is returned when the creation of a Cosmos keyring fails. var ErrCosmosKeyringCreationFailed = errors.New("cosmos keyring creation failed") // ErrCosmosKeyringImportFailed is returned when importing a key into the Cosmos keyring fails. var ErrCosmosKeyringImportFailed = errors.New("cosmos keyring unable to import key") // ... (continue for other error variables)Adding these comments will improve code readability and make it easier for other developers to understand and use these error definitions correctly.
client/keyring/key_config.go (2)
16-28: LGTM: Good use of functional options pattern.The
KeyConfigOpttype andWithKeyFromfunction are well-implemented, using the functional options pattern for flexible configuration. The function correctly handles empty input.Consider adding more robust input validation for the
KeyFromvalue. For example, you might want to check for invalid characters or enforce a specific format for key names.
54-69: Good mnemonic validation, but enhance security measures.The
WithMnemonicfunction includes good practice by validating the mnemonic usingbip39.IsMnemonicValid. However, similar to the previous functions, consider the following enhancements:
- Make the warning about insecurity more prominent.
- Implement a mechanism to prevent this option from being used in production environments.
Here's a suggested improvement:
// WithMnemonic allows to specify a mnemonic phrase as plaintext. // WARNING: This is an insecure option, use for testing only. // The package will create a virtual keyring to derive the keys and meet all the interfaces. func WithMnemonic(v string) KeyConfigOpt { return func(c *cosmosKeyConfig) error { if runtime.GOOS != "test" { return errors.New("WithMnemonic is only allowed in test environments") } if v != "" { if !bip39.IsMnemonicValid(v) { return errors.New("provided mnemonic is not a valid BIP39 mnemonic") } c.Mnemonic = []byte(v) } return nil } }This maintains the good practice of mnemonic validation while adding the production usage safeguard and updating to use
[]bytefor the sensitiveMnemonicfield.client/tm/tmclient.go (1)
94-97: LGTM: Implementation looks good with a minor suggestionThe implementation of
GetStatusis correct and consistent with other methods in the struct. It properly uses the provided context and directly returns the result from the underlying RPC client.Consider expanding the comment slightly to provide more context:
-// GetStatus returns the node status. +// GetStatus returns the status of the Tendermint node, including sync info, validator info, and node info.This gives users a better idea of what information they can expect from the status call.
client/keyring/keyring_config.go (3)
10-18: LGTM: Comprehensive keyring configuration structure.The
cosmosKeyringConfigstruct covers all necessary aspects of keyring configuration, including multiple key management and a default key specification.Consider adding comments to each field to improve code documentation. For example:
type cosmosKeyringConfig struct { // KeyringDir is the directory where the keyring is stored KeyringDir string // KeyringAppName is the name of the application using the keyring KeyringAppName string // KeyringBackend specifies the type of keyring backend to use KeyringBackend Backend // UseLedger indicates whether to use a hardware wallet UseLedger bool // Keys is a slice of key configurations Keys []*cosmosKeyConfig // DefaultKey is the name or address of the default key to use DefaultKey string }
32-72: LGTM: Well-implemented configuration options.The
WithKeyringDir,WithKeyringAppName,WithKeyringBackend, andWithUseLedgerfunctions are well-implemented, following the functional options pattern consistently. The empty string checks in relevant functions prevent setting empty values.Consider adding error returns for invalid inputs, especially for
WithKeyringBackend. For example:func WithKeyringBackend(v Backend) ConfigOpt { return func(c *cosmosKeyringConfig) error { switch v { case BackendTest, BackendFile, BackendOS: c.KeyringBackend = v return nil default: return errors.New("invalid keyring backend") } } }This change would provide better input validation and error handling.
74-108: LGTM: Flexible and robust key addition functions.The
WithKeyandWithNamedKeyfunctions provide a flexible way to add keys to the configuration, with robust error handling for each key option.Consider extracting the common logic of these functions to reduce code duplication:
func addKey(c *cosmosKeyringConfig, name string, opts ...KeyConfigOpt) error { config := &cosmosKeyConfig{Name: name} for optIdx, optFn := range opts { if err := optFn(config); err != nil { return errors.Wrapf(ErrFailedToApplyKeyConfigOption, "key option #%d: %s", optIdx+1, err.Error()) } } c.Keys = append(c.Keys, config) return nil } func WithKey(opts ...KeyConfigOpt) ConfigOpt { return func(c *cosmosKeyringConfig) error { return addKey(c, "", opts...) } } func WithNamedKey(name string, opts ...KeyConfigOpt) ConfigOpt { return func(c *cosmosKeyringConfig) error { return addKey(c, name, opts...) } }This refactoring would reduce duplication and make the code more maintainable.
client/keyring/README.md (3)
1-5: Minor style improvement suggestionThe introduction provides a clear and concise overview of the Injective Chain Keyring Helper. To enhance clarity, consider revising the phrase "a variety of options" on line 3.
You could replace it with "multiple options" or "various options" for a more concise expression:
-Creates a new keyring from a variety of options. See `ConfigOpt` and related options. This keyring helper allows initializing the Cosmos SDK keyring used for signing transactions. +Creates a new keyring from various options. See `ConfigOpt` and related options. This keyring helper allows initializing the Cosmos SDK keyring used for signing transactions.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...ring Helper Creates a new keyring from a variety of options. SeeConfigOptand related op...(A_VARIETY_OF)
84-91: LGTM for Testing section with a suggestionThe Testing section provides clear instructions on how to run the tests and displays the test results. The current coverage of 83.1% is good, but there might be room for improvement.
Consider aiming for higher test coverage, if possible. Identify areas of the code that are not currently covered by tests and add additional test cases to increase the coverage percentage. This will help ensure the reliability and robustness of the keyring helper.
93-101: LGTM for Test Fixture section with a security suggestionThe Generating a Test Fixture section provides clear instructions on how to create a test fixture, including the necessary command and passphrase information. This is helpful for reproducibility in testing environments.
However, for security best practices, consider revising the approach to handling the passphrase:
- Instead of hardcoding the passphrase in the documentation, you could prompt the user to enter a passphrase of their choice when generating the test fixture.
- Alternatively, you could use environment variables to set the passphrase, which would keep it out of the documentation and allow for easier management in different environments.
Example using an environment variable:
export TEST_PASSPHRASE="test12345678" injectived keys --keyring-dir `pwd` --keyring-backend file add testThis approach maintains the ease of use while improving security practices.
go.mod (2)
Line range hint
282-299: Review custom dependency forks and consider upstream alignment.The
replacedirectives point to custom forks of several core dependencies, including:
- cosmos-sdk
- wasmd
- cometbft
- ibc-go
While these custom forks allow for necessary modifications, they also introduce potential risks:
- Divergence from upstream versions may make future upgrades more challenging.
- Custom forks may not receive the same level of community scrutiny as upstream versions.
Consider the following recommendations:
- Regularly review and sync your custom forks with upstream changes to minimize divergence.
- Document the reasons for each custom fork to facilitate future maintenance.
- Implement a process to monitor upstream changes and evaluate their impact on your custom forks.
- Where possible, contribute your changes back to the upstream projects to reduce the need for custom forks.
To help with this process, you can use the following command to list all the replace directives:
#!/bin/bash # List all replace directives grep -n '^replace' go.modThis will help in regularly reviewing and updating these custom dependencies.
Deprecated usages detected in the codebase.
Several deprecated APIs and functions were found across multiple files. These may impact functionality and should be addressed promptly.
Key Locations:
exchange/accounts_rpc/pb/goadesign_goagen_injective_accounts_rpc.pb.goclient/exchange/exchange.goclient/chain/markets_assistant.goclient/chain/chain.goPlease review and update these deprecated usages to ensure compatibility and maintain code quality.
🔗 Analysis chain
Line range hint
1-280: Approve dependency updates and recommend thorough testing.The following key dependencies have been updated:
github.com/cosmos/cosmos-sdkfrom v0.50.7 to v0.50.8github.com/cometbft/cometbftfrom v0.38.9 to v0.38.10github.com/CosmWasm/wasmdfrom v0.40.2 to v0.51.0-inj-0These updates are generally beneficial for security and feature improvements. However, the significant version jump for
wasmdmay introduce breaking changes.Please ensure that comprehensive testing is performed, especially focusing on functionality related to these updated dependencies. Pay extra attention to any CosmWasm-related features due to the major version update of
wasmd.Run the following commands to check for any breaking changes or deprecations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential breaking changes or deprecations rg --type go -i 'deprecated|breaking.?change' $(fd -e go)Length of output: 200
Script:
#!/bin/bash # Check for potential breaking changes or deprecations rg --type go -i 'deprecated|breaking.?change' $(fd -e go)Length of output: 84064
client/keyring/keyring_errors_test.go (10)
10-22: Add documentation comment to test function for clarityConsider adding a descriptive comment to the
TestErrCosmosKeyringCreationFailedfunction to explain its purpose. This will enhance readability and help future maintainers understand the intent of the test.
24-35: Add documentation comment to test function for clarityAdding a brief comment to the
TestErrFailedToApplyConfigOptionfunction will improve code readability and provide context for the test case.
37-58: Add documentation comment to test function for clarityIncluding a descriptive comment for the
TestErrHexFormatErrorfunction will help others understand what specific error scenarios are being tested.
60-82: Add documentation comment to test function for clarityConsider describing the purpose of the
TestErrIncompatibleOptionsProvidedfunction with a comment to enhance maintainability.
84-90: Add documentation comment to test function for clarityAdding a comment to the
TestErrInsufficientKeyDetailsfunction will provide clarity on the test's objective.
92-139: Add documentation comment to test function for clarityIncluding a descriptive comment for the
TestErrKeyIncompatiblefunction will help clarify the intent and improve code readability.
138-138: Address the TODO: Implement test for unsupported multisig keysThe TODO comment indicates that a test for unsupported multisig keys is pending. Implementing this test will enhance error handling coverage and ensure multisig keys are properly tested.
Do you want assistance in implementing this test or opening a GitHub issue to track it?
141-155: Add documentation comment to test function for clarityConsider adding a comment to the
TestErrKeyRecordNotFoundfunction to explain its purpose and the scenario it covers.
157-169: Add documentation comment to test function for clarityProviding a descriptive comment for the
TestErrPrivkeyConflictfunction will improve understanding of the test case.
171-193: Add documentation comment to test function for clarityIncluding a comment for the
TestErrUnexpectedAddressfunction will enhance code clarity and assist future maintainers.client/keyring/keyring.go (2)
399-431: Handle unsupported key types incheckKeyRecordmore gracefully.The function currently returns errors for unsupported key types like
TypeOfflineandTypeMulti. Consider if these key types could be supported or if more descriptive error messages could aid in debugging.Provide clearer guidance or support for additional key types if appropriate.
1-497: Add comments and documentation for exported functions and types.The code lacks GoDoc comments for exported functions and types, which is important for maintaining clear and understandable code, especially in shared libraries.
Add GoDoc comments to all exported functions, methods, and types to improve code readability and maintainability.
🛑 Comments failed to post (12)
.github/workflows/run-tests.yml (1)
20-22:
⚠️ Potential issueReview GPG key generation for security concerns.
The current GPG key generation has some potential security issues:
- The key is generated without passphrase protection (
%no-protection), which could be a security risk if the key is compromised.- The key is set to never expire (
Expire-Date: 0), which is not a best practice for key management.Consider the following improvements:
- Remove the
%no-protectionline to enable passphrase protection. You'll need to securely manage this passphrase, possibly using GitHub Secrets.- Set a reasonable expiration date for the key, e.g.,
Expire-Date: 1yfor one year.- Use GitHub Secrets to store sensitive information like the key configuration.
Here's an example of how you could modify the step:
- name: Generate GPG key env: GPG_KEY_CONFIG: ${{ secrets.GPG_KEY_CONFIG }} run: | echo "$GPG_KEY_CONFIG" > genkey gpg --gen-key --batch genkeyThen, store the key configuration in a GitHub Secret named
GPG_KEY_CONFIG, excluding the%no-protectionline and including a reasonableExpire-Date.client/keyring/key_config.go (2)
8-14: 🛠️ Refactor suggestion
Enhance struct documentation and security.
While the
cosmosKeyConfigstruct effectively encapsulates the necessary information, consider the following improvements:
- Add comments for each field to explain their purpose and usage.
- For sensitive fields (KeyPassphrase, PrivKeyHex, Mnemonic), consider using a more secure type that can be zeroed out after use, such as
[]byte, to minimize the risk of exposing sensitive data in memory.Here's a suggested improvement:
type cosmosKeyConfig struct { Name string // Name of the key configuration KeyFrom string // Source of the key KeyPassphrase []byte // Passphrase for keyring files (sensitive) PrivKeyHex []byte // Private key in hex format (sensitive) Mnemonic []byte // Mnemonic phrase (sensitive) }Remember to update the related functions to handle
[]byteinstead ofstringfor the sensitive fields.
30-52:
⚠️ Potential issueEnhance security measures for sensitive options.
The
WithKeyPassphraseandWithPrivKeyHexfunctions are correctly implemented, but given their sensitive nature, consider the following security enhancements:
- Make the warning about insecurity more prominent, perhaps using a dedicated
WARNING:prefix.- Implement a mechanism to prevent these options from being used in production environments. This could be done through build tags or environment checks.
- For
WithPrivKeyHex, add validation to ensure the input is a valid hex string.Here's an example of how you might implement these suggestions:
// WithKeyPassphrase sets the passphrase for keyring files. // WARNING: This is an insecure option, use for testing only. // The package will fallback to os.Stdin if this option was not provided, but pass is required. func WithKeyPassphrase(v string) KeyConfigOpt { return func(c *cosmosKeyConfig) error { if runtime.GOOS != "test" { return errors.New("WithKeyPassphrase is only allowed in test environments") } if v != "" { c.KeyPassphrase = []byte(v) } return nil } } // WithPrivKeyHex allows to specify a private key as plaintext hex. // WARNING: This is an insecure option, use for testing only. // The package will create a virtual keyring holding that key, to meet all the interfaces. func WithPrivKeyHex(v string) KeyConfigOpt { return func(c *cosmosKeyConfig) error { if runtime.GOOS != "test" { return errors.New("WithPrivKeyHex is only allowed in test environments") } if v != "" { if _, err := hex.DecodeString(v); err != nil { return errors.New("invalid hex string for private key") } c.PrivKeyHex = []byte(v) } return nil } }Note: This example assumes you've updated the
cosmosKeyConfigstruct to use[]bytefor sensitive fields as suggested earlier.client/keyring/README.md (1)
7-36:
⚠️ Potential issueFix typo and LGTM for Usage section
The Usage section provides comprehensive and well-structured information about the
NewCosmosKeyringfunction and its configuration options. There's a minor typo to address:Please apply the following fix:
-* `WithNamedKey` addes a single key to the keyring, with a name. +* `WithNamedKey` adds a single key to the keyring, with a name.Overall, the explanation of options is clear and informative, providing users with a good understanding of how to use the keyring helper.
📝 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.## Usage ```go NewCosmosKeyring(cdc codec.Codec, opts ...ConfigOpt) (sdk.AccAddress, cosmkeyring.Keyring, error) ``` **ConfigOpts:** These options are global on the keyring level. * `WithKeyringDir` option sets keyring path in the filesystem, useful when keyring backend is `file`. * `WithKeyringAppName` option sets keyring application name (defaults to `injectived`) * `WithKeyringBackend` sets the keyring backend. Expected values: `test`, `file`, `os`. * `WithUseLedger` sets the option to use hardware wallet, if available on the system. These options allow adding keys to the keyring during initialization. * `WithKey` adds a single key to the keyring, without having alias name. * `WithNamedKey` adds a single key to the keyring, with a name. * `WithDefaultKey` sets a default key reference to use for signing (by name). **KeyConfigOpts:** These options are set per key. * `WithKeyFrom` sets the key name to use for signing. Must exist in the provided keyring. * `WithKeyPassphrase` sets the passphrase for keyring files. The package will fallback to `os.Stdin` if this option was not provided, but passphrase is required. * `WithPrivKeyHex` allows specifying a private key as plain-text hex. Insecure option, use for testing only. The package will create a virtual keyring holding that key, to meet all the interfaces. * `WithMnemonic` allows specifying a mnemonic phrase as plain-text hex. Insecure option, use for testing only. The package will create a virtual keyring to derive the keys and meet all the interfaces.🧰 Tools
🪛 LanguageTool
[grammar] ~24-~24: Did you mean “having to alias” or “having aliased”?
Context: ...ds a single key to the keyring, without having alias name. *WithNamedKeyaddes a single k...(WITHOUT_VBG_VB)
client/keyring/keyring_test.go (3)
283-286:
⚠️ Potential issueAvoid hardcoding private keys and mnemonics in code
Including hardcoded private keys (
testPrivKeyHex,testOtherPrivKeyHex) and mnemonics (testMnemonic) in the codebase poses a security risk. Even in test code, these sensitive values can be accidentally exposed or misused. Consider generating these values securely at runtime or using environment variables to manage them.🧰 Tools
🪛 Gitleaks
285-285: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
286-286: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
67-67:
⚠️ Potential issueRemove calls to
logPrivKeyto prevent logging private keysThe calls to
logPrivKeyin the test functions log sensitive private key information. Please remove these calls to enhance security.Apply this diff to remove the calls:
// In TestKeyFromPrivkey function - logPrivKey(s.T(), kb, accAddr) // In TestKeyFromMnemonic function - logPrivKey(s.T(), kb, accAddr) // In TestKeyringFile function - logPrivKey(s.T(), kb, accAddr) // In TestNamedKeys function - logPrivKey(s.T(), kb, accAddr)Also applies to: 96-96, 139-139, 270-270
300-304:
⚠️ Potential issueRemove logging of private keys in tests
Logging private keys can lead to unintended exposure of sensitive information if logs are accessible. It's recommended to avoid printing private keys to logs. Please remove the
t.Logstatement to prevent logging the private key.Apply this diff to remove the logging:
func logPrivKey(t *testing.T, kb cosmkeyring.Keyring, accAddr sdk.AccAddress) { armor, _ := kb.ExportPrivKeyArmorByAddress(accAddr, "") privKey, _, _ := cosmcrypto.UnarmorDecryptPrivKey(armor, "") - t.Log("[PRIV]", hex.EncodeToString(privKey.Bytes())) }📝 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 logPrivKey(t *testing.T, kb cosmkeyring.Keyring, accAddr sdk.AccAddress) { armor, _ := kb.ExportPrivKeyArmorByAddress(accAddr, "") privKey, _, _ := cosmcrypto.UnarmorDecryptPrivKey(armor, "") }client/keyring/keyring.go (5)
294-297:
⚠️ Potential issueReconsider using
os.Stdinas the default passphrase reader.Using
os.Stdinmay not be appropriate in all environments, especially where interactive input is not possible or secure.Consider implementing a more flexible passphrase handling mechanism that can accommodate different environments, such as accepting a passphrase from a secure source or providing prompts when appropriate.
144-200: 🛠️ Refactor suggestion
Refactor duplicate logic in
fromPrivkeyHexandfromMnemonic.The functions
fromPrivkeyHexandfromMnemonicshare similar logic for key name handling, address verification, and error handling.Extract the common code into helper functions to reduce duplication and improve maintainability. This will make future updates easier and minimize the risk of inconsistencies.
For example:
func verifyKeyNameAndAddress(keyName string, keyConfig *cosmosKeyConfig, addressFromKey sdk.AccAddress) (string, error) { // Common logic for verifying key name and address }Then update both
fromPrivkeyHexandfromMnemonicto use this helper function.Also applies to: 202-267
490-497:
⚠️ Potential issueAvoid using
panic()and ensure passphrase is suitable for encryption.In the
randPhrasefunction, usingpanic()to handle errors is not recommended in production code. Additionally, converting random bytes directly to a string may result in non-printable characters, which may not be suitable for use as a passphrase.Consider handling the error by returning it and generating a passphrase using a base64 or hex encoding to ensure it contains printable characters.
Apply this diff to improve error handling and passphrase generation:
func randPhrase(size int) (string, error) { buf := make([]byte, size) if _, err := rand.Read(buf); err != nil { - panic(err) + return "", err } - return string(buf) + return hex.EncodeToString(buf), 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 randPhrase(size int) (string, error) { buf := make([]byte, size) if _, err := rand.Read(buf); err != nil { return "", err } return hex.EncodeToString(buf), nil }
469-479:
⚠️ Potential issueHandle the passphrase and errors appropriately in
addFromPrivKey.The function uses a random passphrase to encrypt the private key, but the passphrase is not stored or returned. This means the encrypted key cannot be retrieved later, which might lead to issues if the keyring is used beyond the in-memory session.
Consider returning the passphrase or using a consistent passphrase if the keyring needs to persist beyond the current session. Also, update the function to handle errors from the modified
randPhrasefunction.Apply this diff to address the concerns:
-func addFromPrivKey(kb cosmkeyring.Keyring, name string, privKey cryptotypes.PrivKey) error { - tmpPhrase := randPhrase(64) +func addFromPrivKey(kb cosmkeyring.Keyring, name string, privKey cryptotypes.PrivKey) (string, error) { + tmpPhrase, err := randPhrase(64) + if err != nil { + return "", err + } armored := cosmcrypto.EncryptArmorPrivKey(privKey, tmpPhrase, privKey.Type()) err := kb.ImportPrivKey(name, armored, tmpPhrase) if err != nil { err = errors.Wrapf(ErrCosmosKeyringImportFailed, "failed to import privkey: %s", err.Error()) - return err + return "", err } - return nil + return tmpPhrase, nil }Ensure that any calling functions are updated to handle the returned passphrase and error accordingly.
📝 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 addFromPrivKey(kb cosmkeyring.Keyring, name string, privKey cryptotypes.PrivKey) (string, error) { tmpPhrase, err := randPhrase(64) if err != nil { return "", err } armored := cosmcrypto.EncryptArmorPrivKey(privKey, tmpPhrase, privKey.Type()) err = kb.ImportPrivKey(name, armored, tmpPhrase) if err != nil { err = errors.Wrapf(ErrCosmosKeyringImportFailed, "failed to import privkey: %s", err.Error()) return "", err } return tmpPhrase, nil }
492-494:
⚠️ Potential issueHandle errors from
rand.Readinstead of panicking.Panicking on error is discouraged as it can crash the program and doesn't provide a way to recover or handle the error gracefully.
Modify the function to return an error, allowing the caller to decide how to handle it.
Apply this diff:
func randPhrase(size int) (string, error) { buf := make([]byte, size) if _, err := rand.Read(buf); err != nil { - panic(err) + return "", err } - return string(buf) + return hex.EncodeToString(buf), nil }Committable suggestion was skipped due to low confidence.
proofofze
changed the title
This PR adds a method to
tmClientthat lets callers get the status of the Tendermint node