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 |
Co-authored-by: Marko <marbar3778@yahoo.com>
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
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...e system where the main app is running. This is particularly useful for scenarios wh...
[style] ~48-~48: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...
[uncategorized] ~55-~55: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...e leakage of specific crypto data types expose backward compatibility and extensibilit...
[uncategorized] ~81-~81: The grammatical number of this noun doesn’t look right. Consider replacing it. (AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
Context: ... Prioritize compatibility with previous version to avoid disruptions for existing users...
[style] ~153-~153: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ...irect cryptographic function calls. In all of the interface's methods, we add an *options...
[grammar] ~201-~201: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...` ###### Hasher This interface allows to have a specific hashing algorithm. ```go //...
[style] ~241-~241: Consider replacing this word to strengthen your wording. (AND_THAT)
Context: ...al data. This is a design consideration and may be subject to change during impleme...
[uncategorized] ~615-~615: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ntations ofPrivValidator: *FilePV: Handles file-based private validators. ...
[uncategorized] ~616-~616: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...sed private validators. *SignerClient: Manages remote signing. * `RetrySignerC...
[uncategorized] ~617-~617: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...es remote signing. *RetrySignerClient: Provides retry mechanisms for remote si...
[uncategorized] ~618-~618: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...echanisms for remote signing. *MockPV: Used exclusively for testing purposes. ...
[uncategorized] ~762-~762: You might be missing the article “a” here. (AI_EN_LECTOR_MISSING_DETERMINER_A)
Context: ..., which could store its private keys in Keyring instead of a file in the filesystem. Th...
[uncategorized] ~792-~792: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...urpose of this work allows extensibility so any other primitive can be added in the...
Markdownlint
docs/architecture/adr-071-crypto-v2-multi-curve.md
795-795: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
796-796: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
797-797: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
798-798: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
799-799: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
800-800: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
801-801: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
804-804: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
805-805: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
806-806: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
807-807: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
808-808: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
809-809: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
34-34: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
157-157: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
410-410: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
610-610: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
248-248: Column: 1 (MD010, no-hard-tabs)
Hard tabs
249-249: Column: 1 (MD010, no-hard-tabs)
Hard tabs
250-250: Column: 1 (MD010, no-hard-tabs)
Hard tabs
251-251: Column: 1 (MD010, no-hard-tabs)
Hard tabs
22-22: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
25-25: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
38-38: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
85-85: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
90-90: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
115-115: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
122-122: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
131-131: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
245-245: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
310-310: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
469-469: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
474-474: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
524-524: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
534-534: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
537-537: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
582-582: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
594-594: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
606-606: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
609-609: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
612-612: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
648-648: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
755-755: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
764-764: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
787-787: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
291-291: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
585-585: null (MD034, no-bare-urls)
Bare URL used
586-586: null (MD034, no-bare-urls)
Bare URL used
587-587: null (MD034, no-bare-urls)
Bare URL used
588-588: null (MD034, no-bare-urls)
Bare URL used
589-589: null (MD034, no-bare-urls)
Bare URL used
590-590: null (MD034, no-bare-urls)
Bare URL used
769-769: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
546-546: Expected: asterisk; Actual: underscore (MD049, emphasis-style)
Emphasis style
546-546: Expected: asterisk; Actual: underscore (MD049, emphasis-style)
Emphasis style
Additional comments not posted (7)
docs/architecture/adr-071-crypto-v2-multi-curve.md (7)
48-48: Consider a More Concise Phrasing to Avoid WordinessThe 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] ~48-~48: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...
55-55: Correct the Verb Agreement for ClarityThe 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.Tools
LanguageTool
[uncategorized] ~55-~55: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...e leakage of specific crypto data types expose backward compatibility and extensibilit...
81-81: Adjust the Noun Number for Grammatical AccuracyThe noun "version" should be plural "versions" to correctly refer to multiple previous versions:
- * Prioritize compatibility with previous version to avoid disruptions for existing users. + * Prioritize compatibility with previous versions to avoid disruptions for existing users.Tools
LanguageTool
[uncategorized] ~81-~81: The grammatical number of this noun doesn’t look right. Consider replacing it. (AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
Context: ... Prioritize compatibility with previous version to avoid disruptions for existing users...
153-153: Consider Removing “of” to Be More ConciseThe phrase "all of the interface's methods" can be simplified to enhance readability:
- In all of the interface's methods, we add an *options* input parameter of type `map[string]any`, designed to provide a flexible and dynamic way to pass various options and configurations to the `Sign`, `Verify`, and `Hash` functions. + In all interface methods, we add an *options* input parameter of type `map[string]any`, designed to provide a flexible and dynamic way to pass various options and configurations to the `Sign`, `Verify`, and `Hash` functions.Tools
LanguageTool
[style] ~153-~153: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ...irect cryptographic function calls. In all of the interface's methods, we add an *options...
201-201: Correct Grammatical Structure for ClarityThe phrase "This interface allows to have a specific hashing algorithm" is grammatically incorrect.
- This interface allows to have a specific hashing algorithm. + This interface allows the use of a specific hashingTools
LanguageTool
[grammar] ~201-~201: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...` ###### Hasher This interface allows to have a specific hashing algorithm. ```go //...
241-241: Consider Strengthening the WordingThe phrase "may be subject to change during implementation" can be made more assertive.
- This is a design consideration and may be subject to change during implementation. + This design consideration may change during implementation.Tools
LanguageTool
[style] ~241-~241: Consider replacing this word to strengthen your wording. (AND_THAT)
Context: ...al data. This is a design consideration and may be subject to change during impleme...
795-809: Correct the Indentation for Unordered ListsThe indentation for the list items under "digital signatures" and "Hashing" is inconsistent with markdown best practices.
- * RSA (PSS) - * ECDSA (secp256r1, secp256k1, etc.) - * EdDSA (ed25519, ed448) - * SR25519 - * Schnorr - * Lattice-based (Dilithium) - * BLS (BLS12-381, 377?) + * RSA (PSS) + * ECDSA (secp256r1, secp256k1, etc.) + * EdDSA (ed25519, ed448) + * SR25519 + * Schnorr + * Lattice-based (Dilithium) + * BLS (BLS12-381, 377?)Tools
Markdownlint
795-795: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
796-796: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
797-797: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
798-798: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
799-799: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
800-800: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
801-801: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
804-804: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
805-805: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
806-806: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
807-807: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
808-808: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
809-809: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
There was a problem hiding this comment.
Actionable comments posted: 3
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
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...e system where the main app is running. This is particularly useful for scenarios wh...
[style] ~48-~48: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...
[style] ~153-~153: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ...irect cryptographic function calls. In all of the interface's methods, we add an *options...
[uncategorized] ~187-~187: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ool } ``` ###### Verifier Verifies if given a message belongs to a public key by va...
[grammar] ~201-~201: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...` ###### Hasher This interface allows to have a specific hashing algorithm. ```go //...
[uncategorized] ~229-~229: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...g]any } ``` ###### Public Key Note: Here we decoupled theAddresstype from it...
[style] ~241-~241: Consider replacing this word to strengthen your wording. (AND_THAT)
Context: ...al data. This is a design consideration and may be subject to change during impleme...
[uncategorized] ~260-~260: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...nstead). * Store theProviderMetadatastruct which contains the data that distinguis...
[uncategorized] ~637-~637: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ntations ofPrivValidator: *FilePV: Handles file-based private validators. ...
[uncategorized] ~638-~638: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...sed private validators. *SignerClient: Manages remote signing. * `RetrySignerC...
[uncategorized] ~639-~639: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...es remote signing. *RetrySignerClient: Provides retry mechanisms for remote si...
[uncategorized] ~640-~640: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...echanisms for remote signing. *MockPV: Used exclusively for testing purposes. ...
[uncategorized] ~784-~784: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ..., which could store its private keys in Keyring instead of a file in the filesystem. Th...
[uncategorized] ~814-~814: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...urpose of this work allows extensibility so any other primitive can be added in the...
Markdownlint
docs/architecture/adr-071-crypto-v2-multi-curve.md
817-817: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
818-818: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
819-819: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
820-820: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
821-821: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
822-822: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
823-823: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
826-826: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
827-827: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
828-828: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
829-829: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
830-830: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
831-831: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
34-34: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
157-157: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
419-419: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
632-632: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
248-248: Column: 1 (MD010, no-hard-tabs)
Hard tabs
249-249: Column: 1 (MD010, no-hard-tabs)
Hard tabs
250-250: Column: 1 (MD010, no-hard-tabs)
Hard tabs
251-251: Column: 1 (MD010, no-hard-tabs)
Hard tabs
22-22: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
25-25: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
38-38: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
85-85: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
90-90: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
115-115: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
122-122: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
131-131: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
245-245: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
310-310: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
491-491: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
496-496: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
546-546: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
556-556: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
559-559: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
604-604: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
616-616: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
628-628: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
631-631: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
634-634: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
670-670: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
777-777: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
786-786: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
809-809: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
291-291: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
607-607: null (MD034, no-bare-urls)
Bare URL used
608-608: null (MD034, no-bare-urls)
Bare URL used
609-609: null (MD034, no-bare-urls)
Bare URL used
610-610: null (MD034, no-bare-urls)
Bare URL used
611-611: null (MD034, no-bare-urls)
Bare URL used
612-612: null (MD034, no-bare-urls)
Bare URL used
791-791: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
568-568: Expected: asterisk; Actual: underscore (MD049, emphasis-style)
Emphasis style
568-568: Expected: asterisk; Actual: underscore (MD049, emphasis-style)
Emphasis style
Additional comments not posted (8)
docs/architecture/adr-071-crypto-v2-multi-curve.md (8)
48-48: 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] ~48-~48: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...unit of code organization. ## Context In order to fully understand the need for changes a...
153-153: Consider removing “of” to be more concise.The phrase "all of the interface's methods" can be simplified to enhance readability.
- In all of the interface's methods, we add an *options* input parameter of type `map[string]any`, designed to provide a flexible and dynamic way to pass various options and configurations to the `Sign`, `Verify`, and `Hash` functions. + In all interface methods, we add an *options* input parameter of type `map[string]any`, designed to provide a flexible and dynamic way to pass various options and configurations to the `Sign`, `Verify`, and `Hash` functions.Tools
LanguageTool
[style] ~153-~153: Consider removing “of” to be more concise (ALL_OF_THE)
Context: ...irect cryptographic function calls. In all of the interface's methods, we add an *options...
187-187: Add a comma for clarity in complex sentences.- Verifies if given a message belongs to a public key by validating against its respective signature. + Verifies if, given a message, it belongs to a public key by validating against its respective signature.Tools
LanguageTool
[uncategorized] ~187-~187: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ool } ``` ###### Verifier Verifies if given a message belongs to a public key by va...
229-229: Add a comma for clarity.- *Note:* Here we decoupled the `Address` type from its corresponding `PubKey`. The corresponding codec step is proposed to be abstracted out from the CryptoProvider layer. + *Note:* Here, we decoupled the `Address` type from its corresponding `PubKey`. The corresponding codec step is proposed to be abstracted out from the CryptoProvider layer.Tools
LanguageTool
[uncategorized] ~229-~229: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...g]any } ``` ###### Public Key Note: Here we decoupled theAddresstype from it...
241-241: Consider strengthening the wording.The phrase "may be subject to change during implementation" can be made more assertive.
- This is a design consideration and may be subject to change during implementation. + This design consideration may change during implementation.Tools
LanguageTool
[style] ~241-~241: Consider replacing this word to strengthen your wording. (AND_THAT)
Context: ...al data. This is a design consideration and may be subject to change during impleme...
260-260: Add a comma for clarity.- * Store the `ProviderMetadata` struct which contains the data that distinguishes that provider. + * Store the `ProviderMetadata` struct, which contains the data that distinguishes that provider.Tools
LanguageTool
[uncategorized] ~260-~260: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...nstead). * Store theProviderMetadatastruct which contains the data that distinguis...
637-640: Correct punctuation for list items.The use of colons in list items is inconsistent and could be standardized.
- * `FilePV`: Handles file-based private validators. - * `SignerClient`: Manages remote signing. - * `RetrySignerClient`: Provides retry mechanisms for remote signing. - * `MockPV`: Used exclusively for testing purposes. + * `FilePV` - Handles file-based private validators. + * `SignerClient` - Manages remote signing. + * `RetrySignerClient` - Provides retry mechanisms for remote signing. + * `MockPV` - Used exclusively for testing purposes.Tools
LanguageTool
[uncategorized] ~637-~637: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ntations ofPrivValidator: *FilePV: Handles file-based private validators. ...
[uncategorized] ~638-~638: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...sed private validators. *SignerClient: Manages remote signing. * `RetrySignerC...
[uncategorized] ~639-~639: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...es remote signing. *RetrySignerClient: Provides retry mechanisms for remote si...
[uncategorized] ~640-~640: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...echanisms for remote signing. *MockPV: Used exclusively for testing purposes. ...
817-831: Correct the indentation for unordered lists.- * RSA (PSS) - * ECDSA (secp256r1, secp256k1, etc.) - * EdDSA (ed25519, ed448) - * SR25519 - * Schnorr - * Lattice-based (Dilithium) - * BLS (BLS12-381, 377?) + * RSA (PSS) + * ECDSA (secp256r1, secp256k1, etc.) + * EdDSA (ed25519, ed448) + * SR25519 + * Schnorr + * Lattice-based (Dilithium) + * BLS (BLS12-381, 377?)Tools
Markdownlint
817-817: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
818-818: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
819-819: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
820-820: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
821-821: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
822-822: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
823-823: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
826-826: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
827-827: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
828-828: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
829-829: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
830-830: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
831-831: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
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.