feat: [ADR-071] Cryptography v2 #18824
Conversation
WalkthroughWalkthroughThe new changes propose a significant upgrade to the Cosmos SDK's cryptographic module by introducing multi-curve support. This enhancement allows for the integration of various cryptographic curves for signing and verification. Additionally, the changes provide support for Hardware Security Modules (HSM) and remote signers to improve security and flexibility. Key features include the introduction of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Keyring
participant CryptoProviderFactory
participant CryptoProvider
Client ->> Keyring: ListCryptoProviders()
Keyring -->> Client: List of ProviderMetadata
Client ->> Keyring: GetCryptoProvider(id)
Keyring ->> CryptoProviderFactory: Create(id)
CryptoProviderFactory -->> Keyring: CryptoProvider instance
Keyring -->> Client: CryptoProvider instance
Client ->> CryptoProvider: Perform cryptographic operations
CryptoProvider -->> Client: Operation results
sequenceDiagram
participant System
participant HSM
participant RemoteSigner
System ->> CryptoProvider: Sign(Data)
CryptoProvider ->> HSM: Securely Sign(Data)
HSM -->> CryptoProvider: Signature
CryptoProvider -->> System: Signature
System ->> RemoteSigner: Verify(Data, Signature)
RemoteSigner -->> System: Verification Result
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
tac0turtle
assigned alexanderbez, facundomedica, tac0turtle, julienrbrt and testinginprod
There was a problem hiding this comment.
I've reread this a few times and thought how would the flow work, there are a few things unclear to me.
Is this meant to replace the current crypto package? It seems like a no since modules would all need to be modified to take a cryptoprovider which passes a client and state machine oriented interface to it. It doesnt follow the service design for modules we currently have in the software.
If this is meant to be client side only, does it need to live in the sdk? The only user of this would be CLI users, which I think we should treat as second class citizens, not first.
If this is client side only, then if a chain needs a custom hasher or cipher they would need to reimplement the whole cryptoprovider interface as noops other than what they need. It begs the question, is it better not to have the cryptoprovider and only a bunch of interfaces and services to get them?
It is meant to decouple from the standard and limited crypto package in a way that makes the SDK extensible and allow for more advance cryptography, remote signers, etc.. in a way that is currently not possible or unmaintainable.
It is not meant to be client side only. By having interfaces that decouples behaviour from implementations we follow the SDK's good practices and promote reusability within the ecosystem. Crypto tools are used widely in the SDK and other chains could do even more heavy usage of these tools. So in the future teams can leverage other implementations too in a very modular way without touching any unrelated code.
Actually the |
There was a problem hiding this comment.
Did some thinking about this pr and it should be rewritten into a service based design. There is the cryptoprovider aggregator, that aggregates all the services that are state machine oriented and then there can be another client only crypto provider. Since the state machine does not need GetKeys and potentially other things.
We need to get this ADR to follow existing designs in the sdk with how services work, can be found in "cosmossdk.io/core". Once we have that we should identify what is needed for the state machine. I forsee, GetSigner as the major one as it coincides with future designs we have been thinking about. If you want to chat about the current design in the sdk, let me know.
This adr still needs refining to be able to fit into the sdk in the way you want and the way the sdk is written.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/adr-071-crypto-v2-multi-curve.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/adr-071-crypto-v2-multi-curve.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-071-crypto-v2-multi-curve.md
[typographical] ~17-~17: Consider adding a comma here.
Context: ...f theCryptoProvidersand their design please refer to ADR mentioned above. ## Intro...(PLEASE_COMMA)
[style] ~36-~36: Consider a shorter alternative to avoid wordiness.
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~171-~171: Possible missing preposition found.
Context: ...We will: * Leverage crypto providers * Refactor the module structure as described above...(AI_HYDRA_LEO_MISSING_TO)
Markdownlint
docs/architecture/adr-071-crypto-v2-multi-curve.md
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
23-23: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
26-26: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
73-73: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
76-76: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
81-81: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
99-99: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
137-137: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
142-142: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
161-161: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
186-186: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
196-196: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
199-199: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
Additional comments not posted (19)
docs/architecture/adr-071-crypto-v2-multi-curve.md (19)
23-23: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
23-23: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
26-26: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
26-26: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
9-11: LGTM!The "Status" section is correctly marked as "DRAFT".
17-17: Add a comma for clarity.Consider adding a comma for better readability.
- For in-depth details of the `CryptoProviders` and their design please refer to ADR mentioned above. + For in-depth details of the `CryptoProviders` and their design, please refer to ADR mentioned above.Tools
LanguageTool
[typographical] ~17-~17: Consider adding a comma here.
Context: ...f theCryptoProvidersand their design please refer to ADR mentioned above. ## Intro...(PLEASE_COMMA)
27-33: LGTM!The "Glossary" section is clear and concise.
36-36: Consider a more concise phrasing to avoid wordiness.The phrase "In order to" could be shortened to "To" for conciseness and clarity.
- In order to fully understand the need for changes and the proposed improvements, it's crucial to consider the current state of affairs: + To fully understand the need for changes and the proposed improvements, it's crucial to consider the current state of affairs:Tools
LanguageTool
[style] ~36-~36: Consider a shorter alternative to avoid wordiness.
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...(IN_ORDER_TO_PREMIUM)
73-73: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
73-73: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
76-76: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
76-76: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
81-81: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
81-81: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
99-99: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
99-99: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
137-137: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
137-137: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
142-142: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
142-142: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
162-164: LGTM!The "Alternatives" section is clear and well-written.
161-161: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
161-161: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
186-186: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
186-186: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
196-196: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
196-196: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
199-199: Remove extra blank line.There is an extra blank line that should be removed to improve formatting.
-Tools
Markdownlint
199-199: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
43-43: Correct the verb agreement for clarity.The verb "expose" should be "exposes" to agree with the singular subject "Type leakage".
- Type leakage of specific crypto data types expose backward compatibility and extensibility challenges. + Type leakage of specific crypto data types exposes backward compatibility and extensibility challenges.Likely invalid or redundant comment.
171-171: Add missing preposition.There is a missing preposition in the phrase "Refactor the module structure as described above".
- Refactor the module structure as described above. + Refactor the module structure as described above to.Likely invalid or redundant comment.
Tools
LanguageTool
[uncategorized] ~171-~171: Possible missing preposition found.
Context: ...We will: * Leverage crypto providers * Refactor the module structure as described above...(AI_HYDRA_LEO_MISSING_TO)
|
@tac0turtle can you please take a new look to the ADR please? We got sergio's approve |
#3400) <!-- Please add a reference to the issue that this PR addresses and indicate which files are most critical to review. If it fully addresses a particular issue, please include "Closes #XXX" (where "XXX" is the issue number). If this PR is non-trivial/large/complex, please ensure that you have either created an issue that the team's had a chance to respond to, or had some discussion with the team prior to submitting substantial pull requests. The team can be reached via GitHub Discussions or the Cosmos Network Discord server in the #cometbft channel. GitHub Discussions is preferred over Discord as it allows us to keep track of conversations topically. https://github.com/cometbft/cometbft/discussions If the work in this PR is not aligned with the team's current priorities, please be advised that it may take some time before it is merged - especially if it has not yet been discussed with the team. See the project board for the team's current priorities: https://github.com/orgs/cometbft/projects/1 --> This PR introduces **ADR-117** which implements the `CryptoProvider` [ADR](https://github.com/cosmos/crypto/blob/main/docs/architecture/adr-001-crypto-provider.md) ("aka pluggable cryptography") The ADR in this repo is only a shortened version of the original one posted [here ](cosmos/cosmos-sdk#18824) containing only the relevant concepts and code for cometBFT. For the full text explaining all concepts please check this [file](https://github.com/cosmos/crypto/blob/main/docs/architecture/adr-001-crypto-provider.md) (links are included in ADR-117) --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
Description
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Documentation
New Features
CryptoProviderinterface for signing, verifying, hashing, and metadata retrieval.Signer,Verifier,Hasher,ProviderMetadata,PubKey,PrivKey, andSignature.Refactor
Record.prototo include theCryptoProvidermessage.CryptoProviderFactoryfor creatingCryptoProviders.PrivValidatorimplementations underCryptoProviderPV.