Skip to content
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

feat: new cosmos keyring helper for injective client #235

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

maxim-inj
Copy link
Contributor

@maxim-inj maxim-inj commented Sep 7, 2024

Hi, a little backstory here. Original Cosmos keyring helper was created for Peggo CLI service. It was very cool and really useful, so once we wanted to use the same approach in other apps as well (oracles, etc), we also copied the piece into sdk-go and it sat there for a couple of years.

The fact is that it was never properly tested (unit tests for all branches), it never had interface of possible errors for all failing conditions and it also had a couple of other bugs. In general, the problem with that keyring design was that if the app wants to have 2+ keys loaded at the start time, you can't add them the same way, only using multiple keyrings. Which is not cool.

Last 2 years I was actually reusing the same keyring drop-in function, but during the last year decided to sit down and refactor it, cover with tests and changed the design a bit.

So here is the new version of that keyring helper. I think this PR can be merged as-is once approved, but it will take some time to migrate dependant apps to the new keyring implementation. The latter is not urgent nor necessary. Given that peggo config is very mission critical, I'd advise not change anything in its refactored code.

Feel free to check it out and see the client/keyring/README.md for more details.

Summary by CodeRabbit

  • New Features

    • Introduced comprehensive documentation for the "Injective Chain Keyring Helper," detailing keyring creation and management for the Cosmos SDK.
    • Implemented a keyring management system that supports various key types and configurations, enhancing secure blockchain interactions.
    • Added validation for key configurations, ensuring compatibility and security during keyring operations.
  • Bug Fixes

    • Enhanced error handling for keyring operations, providing clearer messages for various failure scenarios.
  • Tests

    • Added a suite of unit tests for the keyring functionality, validating error handling and key management processes.
  • Chores

    • Updated project dependencies to include new libraries for improved functionality.
    • Refined pre-commit configuration to exclude test data directories from checks.
    • Enhanced GitHub Actions workflow for secure handling of sensitive data during testing.

See client/keyring/README.md for more details.

This comment was marked as off-topic.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Outside diff range, codebase verification and nitpick comments (7)
client/keyring/key_config.go (2)

30-40: Function Implementation Approved with Caution

The WithKeyPassphrase function correctly checks for non-empty strings before setting the KeyPassphrase field. However, it is marked as insecure and for testing only.

Consider adding a warning in the documentation or logs when this function is used, to ensure it is not accidentally used in production environments.


42-52: Function Implementation Approved with Caution

The WithPrivKeyHex function correctly checks for non-empty strings before setting the PrivKeyHex field. It is marked as insecure and for testing only.

Consider adding a warning in the documentation or logs when this function is used, to ensure it is not accidentally used in production environments.

client/keyring/keyring.go (2)

281-351: Function fromCosmosKeyring:

This function integrates with the Cosmos SDK's keyring to fetch or import keys based on the provided configuration. The handling of absolute paths and the use of a custom password reader are notable for enhancing security and flexibility.

Key Points:

  • The function's error handling is comprehensive, providing clear messages for various failure scenarios.
  • The use of a custom io.Reader for password input (passReader) is a thoughtful addition for handling sensitive input securely.

Suggestions:

  • Validate the security implications of handling passwords, especially ensuring that the buffer used in passReader does not expose sensitive data in memory longer than necessary.
  • Consider adding more detailed logging for debugging purposes, especially in error scenarios.

461-473: Function addFromPrivKey:

This function adds a private key to the keyring after encrypting it, which is crucial for maintaining the security of key material in memory and storage.

Key Points:

  • The use of encryption before adding the key to the keyring is a good security practice.
  • Error handling is consistent with the rest of the module, using detailed wrapped errors.

Suggestions:

  • Ensure that the encryption strength and the generated passphrase (randPhrase) are sufficient and align with best security practices.
  • Consider external security audits for cryptographic implementations, especially when handling key material.
client/keyring/README.md (3)

3-3: Consider revising the phrase for clarity.

The phrase "a variety of options" could be simplified to "multiple options" for clarity and conciseness.

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. See ConfigOpt and related op...

(A_VARIETY_OF)


[grammar] ~3-~3: Did you mean “initializing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ted options. This keyring helper allows to initialize Cosmos SDK keyring used for signing tra...

(ALLOW_TO)


24-24: Typographical error in the documentation.

The word "addes" should be corrected to "adds".

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. * WithNamedKey addes a single k...

(WITHOUT_VBG_VB)


62-62: Consider using a hyphen for compound adjectives.

The phrase "Real world use case" should be hyphenated when used as a compound adjective: "Real-world use case".

Tools
LanguageTool

[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b7a68cc and d60376e.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (13)
  • client/keyring/README.md (1 hunks)
  • client/keyring/errors.go (1 hunks)
  • client/keyring/key_config.go (1 hunks)
  • client/keyring/keyring.go (1 hunks)
  • client/keyring/keyring_config.go (1 hunks)
  • client/keyring/keyring_errors_test.go (1 hunks)
  • client/keyring/keyring_test.go (1 hunks)
  • client/keyring/testdata/keyring-file/263117aad9ebf5759a8272ad2ae4968dd12d4602.address (1 hunks)
  • client/keyring/testdata/keyring-file/310322fcb0937ade77a9ba0d128f9e7f17312796.address (1 hunks)
  • client/keyring/testdata/keyring-file/keyhash (1 hunks)
  • client/keyring/testdata/keyring-file/test.info (1 hunks)
  • client/keyring/testdata/keyring-file/test2.info (1 hunks)
  • go.mod (5 hunks)
Additional context used
LanguageTool
client/keyring/README.md

[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. See ConfigOpt and related op...

(A_VARIETY_OF)


[grammar] ~3-~3: Did you mean “initializing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ted options. This keyring helper allows to initialize Cosmos SDK keyring used for signing tra...

(ALLOW_TO)


[uncategorized] ~5-~5: Possible missing preposition found.
Context: ...ed for signing transactions. It allows flexibly define a static configuration of keys, ...

(AI_HYDRA_LEO_MISSING_TO)


[grammar] ~5-~5: Did you mean “loading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ned keys in the same keyring and allows to load keys from a file, derive from mnemonic ...

(ALLOW_TO)


[grammar] ~22-~22: Did you mean “adding”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ble on the system. These options allow to add keys to the keyring during initializati...

(ALLOW_TO)


[grammar] ~24-~24: Did you mean “having to alias” or “having aliased”?
Context: ...ds a single key to the keyring, without having alias name. * WithNamedKey addes a single k...

(WITHOUT_VBG_VB)


[grammar] ~34-~34: Did you mean “specifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... is required. * WithPrivKeyHex allows to specify a private key as plain-text hex. Insecu...

(ALLOW_TO)


[grammar] ~35-~35: Did you mean “specifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...the interfaces. * WithMnemonic allows to specify a mnemonic pharse as plain-text hex. In...

(ALLOW_TO)


[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Gitleaks
client/keyring/keyring_test.go

283-283: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


284-284: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

Additional comments not posted (31)
client/keyring/testdata/keyring-file/keyhash (1)

1-1: Clarify security measures for sensitive data storage.

The file contains a bcrypt hash, which is typically used for secure storage of sensitive data. Please clarify the security measures in place to protect this file, and confirm its intended use within the system.

client/keyring/testdata/keyring-file/263117aad9ebf5759a8272ad2ae4968dd12d4602.address (1)

1-1: Discuss the lifecycle management of JWE tokens.

The file contains a JWE token, which is used for secure data transmission. Please provide details on the lifecycle management of these tokens, including their expiration, renewal, and revocation processes to ensure they are handled securely.

client/keyring/testdata/keyring-file/310322fcb0937ade77a9ba0d128f9e7f17312796.address (1)

1-1: Ensure consistent security practices for JWE tokens.

This file, similar to the previous one, contains a JWE token. Ensure that security practices, including token lifecycle management, are consistently applied across all instances to maintain the integrity and confidentiality of the data.

client/keyring/testdata/keyring-file/test.info (1)

1-1: Security Implications of Encrypted Data

This file contains encrypted data, which is typically used for testing. Ensure that this data does not contain any sensitive or real user information to avoid potential security risks.

client/keyring/testdata/keyring-file/test2.info (1)

1-1: Security Implications of Encrypted Data

This file, like the previous one, contains encrypted data used for testing. It is crucial to ensure that no sensitive or real user information is included in this data to prevent security risks.

client/keyring/key_config.go (3)

8-14: Struct Definition Approved

The cosmosKeyConfig struct is well-defined and covers all necessary fields for key configuration.


19-28: Function Implementation Approved

The WithKeyFrom function correctly checks for non-empty strings before setting the KeyFrom field.


54-69: Function Implementation Approved

The WithMnemonic function correctly checks for non-empty strings and validates the mnemonic using the BIP39 standard before setting the Mnemonic field.

client/keyring/keyring_errors_test.go (9)

10-22: Test for Cosmos Keyring Creation Failure: Approved

The test function TestErrCosmosKeyringCreationFailed correctly checks for the ErrCosmosKeyringCreationFailed error when an invalid backend is specified. This is a good practice for ensuring robust error handling in the keyring helper.


24-35: Test for Failed Configuration Option: Approved

The test function TestErrFailedToApplyConfigOption correctly checks for the ErrFailedToApplyConfigOption error when an invalid configuration option is provided. This ensures that the keyring helper properly handles configuration errors.


37-58: Test for Hexadecimal Format Error: Approved

The test function TestErrHexFormatError correctly checks for the ErrHexFormatError error when a non-hexadecimal private key is provided. This is crucial for ensuring that the keyring helper correctly validates private key formats.


60-82: Test for Incompatible Options: Approved

The test function TestErrIncompatibleOptionsProvided correctly checks for the ErrIncompatibleOptionsProvided error when incompatible options are used together. This helps ensure that the keyring helper enforces compatibility rules among options.


84-90: Test for Insufficient Key Details: Approved

The test function TestErrInsufficientKeyDetails correctly checks for the ErrInsufficientKeyDetails error when no key details are provided. This is essential for ensuring that the keyring helper requires necessary key information.


92-138: Test for Key Incompatibility: Approved

The test function TestErrKeyIncompatible correctly checks for the ErrKeyIncompatible error when an incompatible key is used with the keyring. This test is crucial for ensuring that the keyring helper handles key compatibility issues effectively.


141-155: Test for Key Record Not Found: Approved

The test function TestErrKeyRecordNotFound correctly checks for the ErrKeyRecordNotFound error when a key record cannot be found. This is important for ensuring that the keyring helper handles cases where the specified key does not exist.


157-169: Test for Private Key Conflict: Approved

The test function TestErrPrivkeyConflict correctly checks for the ErrPrivkeyConflict error when there is a conflict between the private key and the mnemonic. This test is essential for ensuring that the keyring helper correctly handles conflicts in key information.


171-193: Test for Unexpected Address: Approved

The test function TestErrUnexpectedAddress correctly checks for the ErrUnexpectedAddress error when an address does not match the expected private key or mnemonic. This is vital for ensuring that the keyring helper accurately validates addresses against key information.

client/keyring/keyring.go (2)

1-19: Review of package and imports:

The package declaration and imports are appropriate for the functionality described. The use of specific libraries like github.com/cosmos/cosmos-sdk/crypto/keyring and github.com/pkg/errors aligns with the needs of cryptographic operations and enhanced error handling. Ensure that all these libraries are up-to-date and maintained.


21-24: Global variables:

The use of global variables for default values (defaultKeyringKeyName, emptyCosmosAddress) is acceptable here as they provide a clear, centralized way to manage defaults. However, consider making emptyCosmosAddress a constant if it does not change.

client/keyring/keyring_config.go (7)

7-18: Type Definitions Approved

The ConfigOpt function type and cosmosKeyringConfig struct are well-defined and align with Go's idiomatic approach to configuring objects using functional options.


20-30: Backend Type and Constants Approved

The Backend type alias and its constants (BackendTest, BackendFile, BackendOS) are clearly defined, enhancing readability and type safety in the configuration of keyring backends.


43-52: Function Implementation Approved

The function WithKeyringAppName is implemented correctly, ensuring that an empty application name is not set, which aligns with the expected functionality.


65-72: Function Implementation Approved

The function WithUseLedger is implemented correctly, setting the hardware wallet usage option based on the provided boolean value without unnecessary checks.


74-89: Good Error Handling in Key Configuration

The function WithKey effectively applies key configuration options and handles errors appropriately. The use of errors.Wrapf to provide detailed error messages enhances the debuggability of the keyring configuration process.


91-108: Named Key Configuration Approved

The function WithNamedKey correctly applies named key configurations and handles errors effectively, similar to the WithKey function. The consistent error handling approach is commendable.


110-120: Function Implementation Approved

The function WithDefaultKey is implemented correctly, ensuring that an empty default key name is not set, which aligns with the expected functionality.

go.mod (5)

162-162: New Dependency: github.com/minio/highwayhash

The addition of github.com/minio/highwayhash as an indirect dependency. This library provides a fast hash function that is useful for hash tables, message authentication, and other data processing tasks. Ensure that the project uses this hash function for appropriate tasks where performance is critical, and verify its integration and usage within the project.

Verification successful

Indirect Dependency Confirmed: github.com/minio/highwayhash

The github.com/minio/highwayhash library is confirmed as an indirect dependency, as it appears in the go.sum file. However, there are no direct references or imports of this library in the project's Go code, indicating it is used by another dependency rather than directly by the project. No further action is required regarding its direct integration or usage.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `github.com/minio/highwayhash` in the project's code.

# Test: Search for the import statement in the project's Go files. Expect: At least one occurrence.
rg --type go 'import.*github.com/minio/highwayhash'

Length of output: 53


Script:

#!/bin/bash
# Check if `github.com/minio/highwayhash` is listed in the go.sum file
rg 'github.com/minio/highwayhash' go.sum

# Search for any references to `highwayhash` in the codebase
rg 'highwayhash' --type go

Length of output: 242


21-21: Dependency Change: Direct Usage of github.com/cosmos/go-bip39

The dependency github.com/cosmos/go-bip39 has been changed from indirect to direct usage. This change suggests that the module is now explicitly used in the project's code rather than being pulled in by another dependency. Ensure that this change aligns with the project's requirements for handling cryptographic functions, especially in the context of key management.

Verification successful

Verified Direct Usage of github.com/cosmos/go-bip39

The dependency github.com/cosmos/go-bip39 is directly used in the codebase, specifically in client/keyring/key_config.go. This confirms the appropriateness of changing it from indirect to direct in the go.mod file. No further action is needed.

  • client/keyring/key_config.go: Directly imports and uses github.com/cosmos/go-bip39.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the direct usage of `github.com/cosmos/go-bip39` in the project's code.

# Test: Search for the import statement in the project's Go files. Expect: At least one occurrence.
rg --type go 'import.*github.com/cosmos/go-bip39'

Length of output: 51


Script:

#!/bin/bash
# Description: Search for any usage of `bip39` functions or types in the project's Go files.

# Test: Search for any usage of `bip39` in the project's Go files. Expect: At least one occurrence if used.
rg --type go 'bip39'

Length of output: 421


155-155: New Dependency: github.com/lib/pq

The addition of github.com/lib/pq as an indirect dependency. This library is a pure Go Postgres driver for the database/sql package. Given the project's potential need for database interactions, particularly with Postgres, this addition seems appropriate. However, confirm that this aligns with the project's database strategy and that there are actual implementations using this driver.


140-140: New Dependency: github.com/hashicorp/golang-lru/v2

The addition of github.com/hashicorp/golang-lru/v2 as an indirect dependency. This library provides an LRU cache implementation. It's important to ensure that this caching mechanism is used appropriately across the project, especially in scenarios where caching can lead to significant performance improvements.

Verification successful

Indirect Dependency Not Directly Used

The github.com/hashicorp/golang-lru/v2 library is not directly imported or used in the codebase. It appears as an indirect dependency, likely due to another package's requirements. Therefore, there is no need to ensure its usage across the project. If further verification is needed, consider manually checking the go.mod file for dependencies that might indirectly use this library.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `github.com/hashicorp/golang-lru/v2` in the project's code.

# Test: Search for the import statement in the project's Go files. Expect: At least one occurrence.
rg --type go 'import.*github.com/hashicorp/golang-lru/v2'

Length of output: 59


Script:

#!/bin/bash
# Search for direct dependencies in go.mod that might use `github.com/hashicorp/golang-lru/v2` indirectly.
rg 'require ' go.mod

# Search for LRU cache usage patterns in the codebase to see if the functionality is being used indirectly.
rg --type go 'LRU|lru|cache'

Length of output: 293


126-126: New Dependency: github.com/google/orderedcode

The addition of github.com/google/orderedcode as an indirect dependency. This library is typically used for encoding ordered lists of primitive types into byte slices that sort lexically. Review the use cases within the project that require such functionality, particularly if it relates to database key encoding or similar scenarios.

Verification successful

Indirect Dependency Confirmed: github.com/google/orderedcode

The library github.com/google/orderedcode is confirmed as an indirect dependency in the project. It is not directly imported or used in the codebase, indicating it is likely a sub-dependency of another package. No direct impact on the project's functionality has been identified.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `github.com/google/orderedcode` in the project's code.

# Test: Search for the import statement in the project's Go files. Expect: At least one occurrence.
rg --type go 'import.*github.com/google/orderedcode'

Length of output: 54


Script:

#!/bin/bash
# Search for any direct dependencies in go.mod that might include `github.com/google/orderedcode` as a sub-dependency.
rg 'require ' go.mod

# Search for any usage of functions or types from the `orderedcode` package in the codebase.
rg 'orderedcode\.' --type go

Length of output: 73


Script:

#!/bin/bash
# Check the go.mod file for any comments or indirect dependencies related to `orderedcode`.
rg 'orderedcode' go.mod

# Inspect the go.sum file to see if `orderedcode` is listed, confirming its presence as an indirect dependency.
rg 'orderedcode' go.sum

Length of output: 274

client/keyring/errors.go Show resolved Hide resolved
client/keyring/keyring.go Outdated Show resolved Hide resolved
client/keyring/keyring.go Outdated Show resolved Hide resolved
client/keyring/keyring.go Show resolved Hide resolved
client/keyring/keyring_config.go Outdated Show resolved Hide resolved
client/keyring/keyring_config.go Show resolved Hide resolved
client/keyring/README.md Outdated Show resolved Hide resolved
client/keyring/keyring_test.go Show resolved Hide resolved
@aarmoa
Copy link
Collaborator

aarmoa commented Sep 9, 2024

Hello @maxim-inj. I will start reviewing the PR. In the meantime, could you please do the following?

  • Point the PR to the dev branch. Unless we are working on a urgent fix, all PRs should be created pointing to dev
  • Make sure that both the pre-commit and run-tests workflows pass in green

Copy link
Collaborator

@aarmoa aarmoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @maxim-inj, thank you for adding this new implementation to the SDK.
I think it is great that the new keyring is in a new module, so that we can support the old and new implementations in parallel. Once the PR is merged we should create a task to migrate all repo examples to use the new keyring, and also to mark the old keyring creation logic as deprecated.

I will be happy to approve the PR once all the validation workflows pass in green

@maxim-inj maxim-inj changed the base branch from master to dev September 9, 2024 14:12
@maxim-inj
Copy link
Contributor Author

Unless we are working on a urgent fix, all PRs should be created pointing to dev

Oh didn't know, indeed. Rebased, will wait for CI to pass.

Once the PR is merged we should create a task to migrate all repo examples to use the new keyring, and also to mark the old keyring creation logic as deprecated.

Yeah this will be an ongoning process with low priority.

@aarmoa
Copy link
Collaborator

aarmoa commented Sep 11, 2024

@maxim-inj still waiting for you to resolve all issues in the validation workflows before approving the PR

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e009b64 and 20919a1.

Files selected for processing (2)
  • .pre-commit-config.yaml (1 hunks)
  • client/keyring/README.md (1 hunks)
Files skipped from review due to trivial changes (1)
  • .pre-commit-config.yaml
Additional context used
LanguageTool
client/keyring/README.md

[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. See ConfigOpt and related op...

(A_VARIETY_OF)


[grammar] ~3-~3: Did you mean “initializing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ted options. This keyring helper allows to initialize Cosmos SDK keyring used for signing tra...

(ALLOW_TO)


[uncategorized] ~5-~5: Possible missing preposition found.
Context: ...ed for signing transactions. It allows flexibly define a static configuration of keys, ...

(AI_HYDRA_LEO_MISSING_TO)


[grammar] ~5-~5: Did you mean “loading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ned keys in the same keyring and allows to load keys from a file, derive from mnemonic ...

(ALLOW_TO)


[grammar] ~22-~22: Did you mean “adding”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ble on the system. These options allow to add keys to the keyring during initializati...

(ALLOW_TO)


[grammar] ~24-~24: Did you mean “having to alias” or “having aliased”?
Context: ...ds a single key to the keyring, without having alias name. * WithNamedKey addes a single k...

(WITHOUT_VBG_VB)


[grammar] ~34-~34: Did you mean “specifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... is required. * WithPrivKeyHex allows to specify a private key as plain-text hex. Insecu...

(ALLOW_TO)


[grammar] ~35-~35: Did you mean “specifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...the interfaces. * WithMnemonic allows to specify a mnemonic pharse as plain-text hex. In...

(ALLOW_TO)


[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Additional comments not posted (2)
client/keyring/README.md (2)

37-82: LGTM!

The Examples section provides clear and easy-to-understand code examples on how to use the keyring helper. It covers different use cases such as initializing an in-memory keyring with a private key hex, initializing an in-memory keyring with a mnemonic phrase, and a real-world use case of keyring initialization from CLI flags with a single named key set as default.

No issues were flagged by the static analysis tool in this section.

Tools
LanguageTool

[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


84-101: LGTM!

The Testing and Generating a Test Fixture sections provide useful information for developers working with the keyring helper. The instructions are clear and easy to follow.

The Testing section shows how to run the tests for the keyring helper and the expected output. The Generating a Test Fixture section provides step-by-step instructions on how to generate a test fixture for the keyring helper, including the commands to run and the passphrase to use.

No issues were flagged by the static analysis tool in these sections.

client/keyring/README.md Outdated Show resolved Hide resolved
client/keyring/README.md Outdated Show resolved Hide resolved
client/keyring/README.md Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 20919a1 and 973cf13.

Files selected for processing (3)
  • .github/workflows/run-tests.yml (1 hunks)
  • client/keyring/keyring_errors_test.go (1 hunks)
  • client/keyring/keyring_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • client/keyring/keyring_errors_test.go
Additional context used
Gitleaks
client/keyring/keyring_test.go

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)

Additional comments not posted (10)
.github/workflows/run-tests.yml (3)

18-19: LGTM!

Installing the pass utility is a good practice for securely managing sensitive data in the workflow. The step uses sudo to install the package, which is appropriate for a GitHub Actions workflow.


20-22: LGTM!

Generating a GPG key allows for secure handling of sensitive data in the workflow. The configuration seems appropriate for generating a secure GPG key. The step uses a heredoc to provide the configuration, which is a good practice for readability.


23-24: LGTM!

Initializing the password store is necessary for using the pass utility installed in the previous step. The name keyring_test seems appropriate for a test workflow.

client/keyring/keyring_test.go (7)

53-53: The previous review comment about the security risk of exposing private keys is still valid.


82-82: The previous review comment about the security risk of exposing private keys is still valid.


104-145: LGTM!

The code changes are approved.


147-214: LGTM!

The code changes are approved.


222-222: The previous review comment about the security risk of exposing private keys is still valid.


249-249: The previous review comment about the security risk of exposing private keys is still valid.

Also applies to: 254-254


280-298: The previous review comment and static analysis hints about the security risk of exposing private keys are still valid.

Consider using environment variables or a separate configuration file for sensitive data.

To mitigate the security risk, consider storing sensitive data such as private keys, mnemonic phrases, and signatures in environment variables or a separate configuration file that is not checked into version control.

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)

client/keyring/keyring_test.go Show resolved Hide resolved
@maxim-inj
Copy link
Contributor Author

@aarmoa alright it's good to go!

Copy link
Collaborator

@aarmoa aarmoa left a 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

@aarmoa aarmoa merged commit 2c8f9a0 into dev Sep 18, 2024
4 checks passed
@aarmoa aarmoa deleted the f/new-cosmos-keyring branch September 18, 2024 03:28
@coderabbitai coderabbitai bot mentioned this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants