refactor!: (partially) Decouple cometbft/crypto deps #20748
Conversation
Adapt to use cosmos/crypto [WIP]
# Conflicts: # crypto/codec/cmt.go # go.mod # store/internal/maps/maps.go
WalkthroughWalkthroughThe recent changes across various files focus on transitioning cryptographic dependencies from Changes
Sequence DiagramsSilently skipped generating sequence diagrams as the changes are straightforward and do not introduce complex control flows or new features requiring visualization. Possibly related issues
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 (
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/go.sumis excluded by!**/*.sum
Files selected for processing (2)
- simapp/go.mod (2 hunks)
- x/auth/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- simapp/go.mod
- x/auth/go.mod
raynaudoe
changed the title
raynaudoe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
crypto/armor.go (2)
63-68: Add a doc comment forEncodeArmor.The function lacks a doc comment. Adding one will improve readability and maintainability.
// EncodeArmor encodes the given data into an armored string format. // It takes a block type, headers, and data as input and returns the armored string and any error encountered.
70-75: Add a doc comment forDecodeArmor.The function lacks a doc comment. Adding one will improve readability and maintainability.
// DecodeArmor decodes an armored string into its components. // It returns the block type, headers, decoded data, and any error encountered during decoding.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- baseapp/baseapp.go (2 hunks)
- crypto/armor.go (9 hunks)
- crypto/armor_test.go (8 hunks)
- store/go.mod (1 hunks)
- x/auth/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- store/go.mod
- x/auth/go.mod
Additional context used
Path-based instructions (3)
crypto/armor.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/armor_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (13)
crypto/armor.go (6)
Line range hint
100-111:
LGTM!The function is correct and handles errors properly.
Line range hint
113-132:
LGTM!The function is correct and handles errors properly.
Line range hint
143-152:
LGTM!The function is correct and handles errors properly.
Line range hint
196-228:
LGTM!The function is correct and handles errors properly.
Line range hint
230-265:
LGTM!The function is correct and handles errors properly.
Line range hint
265-288:
LGTM!The function is correct and handles errors properly.
crypto/armor_test.go (6)
73-75: LGTM!The function is correct and handles errors properly.
129-131: LGTM!The function is correct and handles errors properly.
141-143: LGTM!The function is correct and handles errors properly.
155-157: LGTM!The function is correct and handles errors properly.
183-185: LGTM!The function is correct and handles errors properly.
242-244: LGTM!The function is correct and handles errors properly.
baseapp/baseapp.go (1)
699-701: LGTM!The function is correct and handles errors properly.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- crypto/armor.go (8 hunks)
- crypto/keyring/keyring.go (15 hunks)
Files skipped from review as they are similar to previous changes (1)
- crypto/armor.go
Additional context used
Path-based instructions (1)
crypto/keyring/keyring.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (17)
crypto/keyring/keyring.go (17)
25-25: Update import alias and path.The import alias for
typeswas renamed tosdkcrypto. Ensure that all references totypesare updated tosdkcrypto.
98-98: Update function signature to usesdkcrypto.PubKey.The function signature was updated to use
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
101-101: Update function signature to usesdkcrypto.PubKey.The function signature was updated to use
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
114-114: Update function signature to usesdkcrypto.PubKey.The function signature was updated to use
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
117-117: Update function signature to usesdkcrypto.PubKey.The function signature was updated to use
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
159-159: Update function signature to usesdkcrypto.PubKey.The function signature was updated to use
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
303-303: Update function signature to usesdkcrypto.PrivKey.The function signature was updated to use
sdkcrypto.PrivKey. Ensure that the new type is correctly used throughout the function.
307-307: Update function signature to usesdkcrypto.PrivKey.The function signature was updated to use
sdkcrypto.PrivKey. Ensure that the new type is correctly used throughout the function.
383-383: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
396-396: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
429-429: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
453-453: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
462-462: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
466-466: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
810-810: Update function to usesdkcrypto.PrivKey.The function uses
sdkcrypto.PrivKey. Ensure that the new type is correctly used throughout the function.
897-897: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
907-907: Update function to usesdkcrypto.PubKey.The function uses
sdkcrypto.PubKey. Ensure that the new type is correctly used throughout the function.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- crypto/armor_test.go (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- crypto/armor_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- crypto/keyring/keyring.go (15 hunks)
- simapp/go.mod (2 hunks)
- store/go.mod (1 hunks)
- x/auth/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- crypto/keyring/keyring.go
- simapp/go.mod
- store/go.mod
Additional comments not posted (1)
x/auth/go.mod (1)
20-20: Dependency Update: Verify compatibility and stability.The addition of
github.com/cosmos/crypto v0.1.1aligns with the PR's objective to decouple fromcometbft/crypto. Ensure that this version is compatible and stable for your use case.
There was a problem hiding this comment.
pretty confused on what the plan with the crypto repo is, it seems like all the interfaces and code was moved there, absorbing tech debt instead of starting fresh and figuring out how to migrate repos to that repo. Could an explanation of what the plan is here? it seems like a lot of changes for no benefit at first glance
| func EncodeArmor(blockType string, headers map[string]string, data []byte) string { | ||
| buf := new(bytes.Buffer) | ||
| w, err := armor.Encode(buf, blockType, headers) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not encode ascii armor: %w", err)) | ||
| } | ||
| _, err = w.Write(data) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not encode ascii armor: %w", err)) | ||
| } | ||
| err = w.Close() | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not encode ascii armor: %w", err)) | ||
| } | ||
| return buf.String() | ||
| } | ||
|
|
||
| func DecodeArmor(armorStr string) (blockType string, headers map[string]string, data []byte, err error) { | ||
| buf := bytes.NewBufferString(armorStr) | ||
| block, err := armor.Decode(buf) | ||
| if err != nil { | ||
| return "", nil, nil, err | ||
| } | ||
| data, err = io.ReadAll(block.Body) | ||
| if err != nil { | ||
| return "", nil, nil, err | ||
| } | ||
| return block.Type, block.Header, data, nil | ||
| } |
There was a problem hiding this comment.
is this needed in crypto? if we just migrated all of the things from sdk to that repo then i think you will be starting with tech debt right away. I dont think this is a good plan and the repo should start fresh with better standards.
| @@ -36,11 +36,11 @@ func (sm *merkleMap) set(key string, value []byte) { | |||
|
|
|||
| // The value is hashed, so you can check for equality with a cached value (say) | |||
| // and make a determination to fetch or not. | |||
| vhash := sha256.Sum256(value) | |||
There was a problem hiding this comment.
why migrate to cosmos crypto here?
There was a problem hiding this comment.
because the sha256 code was a copy&paste of what cometbft has. The idea is that also comet uses cosmos/crypto for the common crypto code.
There was a problem hiding this comment.
but why not use sha256 directly instead of a wrapper?
| VerifySignature(msg, sig []byte) bool | ||
| Equals(PubKey) bool | ||
| Type() string | ||
| cosmoscrypto.PubKey |
There was a problem hiding this comment.
i dont follow why these interfaces were moved? wont you have new apis?
There was a problem hiding this comment.
I've found a way to avoid having the Pubkey and Privkey ifaces copied across codebases. So in cosmos/crypto we have the "vanilla" ifaces, then in the sdk we can extend the ifaces to match the local requirements (mostly to be compatible with gogoproto). Same thing can be done is cometbft
This PR's primary goal is to decouple as much code as possible from |
# Conflicts: # CHANGELOG.md # crypto/keyring/record.go # crypto/keys/multisig/multisig.go # simapp/go.mod # x/auth/go.mod
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (5)
crypto/keys/multisig/multisig.go (4)
Line range hint
24-37:
Improve error handling inNewLegacyAminoPubKey.The constructor
NewLegacyAminoPubKeyusespanicfor error handling, which is generally discouraged in Go unless it's a developer error that can't be recovered. Consider returning an error instead to allow for more graceful error handling.- func NewLegacyAminoPubKey(threshold int, pubKeys []sdkcryptotypes.PubKey) *LegacyAminoPubKey { + func NewLegacyAminoPubKey(threshold int, pubKeys []sdkcryptotypes.PubKey) (*LegacyAminoPubKey, error) { if threshold <= 0 { - panic("threshold k of n multisignature: k <= 0") + return nil, errors.New("threshold k of n multisignature: k <= 0") } if len(pubKeys) < threshold { - panic("threshold k of n multisignature: len(pubKeys) < k") + return nil, errors.New("threshold k of n multisignature: len(pubKeys) < k") } anyPubKeys, err := packPubKeys(pubKeys) if err != nil { - panic(err) + return nil, err } - return &LegacyAminoPubKey{Threshold: uint32(threshold), PubKeys: anyPubKeys} + return &LegacyAminoPubKey{Threshold: uint32(threshold), PubKeys: anyPubKeys}, nil }
Line range hint
123-135:
Add missing import forcosmoscrypto.The function uses
cosmoscrypto.PubKey, but the import forcosmoscryptois missing, causing a compilation error.+ "github.com/cosmos/crypto"Tools
GitHub Check: tests (03)
[failure] 123-123:
undefined: cosmoscryptoGitHub Check: tests (02)
[failure] 123-123:
undefined: cosmoscryptoGitHub Check: tests (01)
[failure] 123-123:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscryptoGitHub Check: tests (00)
[failure] 123-123:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscryptoGitHub Check: dependency-review
[failure] 123-123:
undefined: cosmoscrypto
Line range hint
155-160:
Improve error handling inUnpackInterfaces.The method should handle errors gracefully instead of potentially propagating them up the call stack without adequate context or logging. Consider enhancing error handling with additional context or logging to aid in debugging.
if err != nil { - return err + return fmt.Errorf("failed to unpack interface: %w", err) }
Line range hint
164-172:
Ensure type safety inpackPubKeys.The method
packPubKeysattempts to set a value that might not be compatible with the expected type, which could lead to runtime errors. Ensure that the type conversions and assignments are safe and appropriate for the expected types.- any, err := types.NewAnyWithValue(pubKeys[i]) + any, err := types.NewAnyWithValue(pubKeys[i].(proto.Message))CHANGELOG.md (1)
45-45: The changelog entry may be inaccurate.The search results show occurrences of both
cometbft/cryptoandcosmos/cryptoin the codebase, suggesting that the replacement might not be complete or that both are used concurrently. Verify the specific changes in the codebase to determine if the changelog entry is accurate.
cometbft/cryptois found in several files.cosmos/cryptois also found in several files.#!/bin/bash # Description: Verify the imports and usage of `cometbft/crypto` and `cosmos/crypto` in the codebase. # Search for import statements of `cometbft/crypto` rg --type go 'import.*cometbft/crypto' -A 5 # Search for import statements of `cosmos/crypto` rg --type go 'import.*cosmos/crypto' -A 5Analysis chain
LGTM! Verify the accuracy of the changelog entry.
The changelog entry is clear and concise. Ensure that it accurately reflects the changes made in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the changelog entry accurately reflects the changes made in the codebase. # Test: Search for the usage of `cometbft/crypto` and `cosmos/crypto` in the codebase. Expect: No occurrences of `cometbft/crypto` and occurrences of `cosmos/crypto`. rg --type go -l 'cometbft/crypto' rg --type go -l 'cosmos/crypto'Length of output: 2846
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
store/go.sumis excluded by!**/*.sum
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (2 hunks)
- crypto/keyring/record.go (7 hunks)
- crypto/keys/multisig/multisig.go (7 hunks)
- simapp/go.mod (1 hunks)
- store/go.mod (1 hunks)
- store/internal/maps/maps.go (3 hunks)
Additional context used
Path-based instructions (5)
crypto/keyring/record.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/internal/maps/maps.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/keys/multisig/multisig.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
GitHub Check: tests (03)
crypto/keys/multisig/multisig.go
[failure] 40-40:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
GitHub Check: tests (02)
crypto/keys/multisig/multisig.go
[failure] 40-40:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
GitHub Check: tests (01)
crypto/keys/multisig/multisig.go
[failure] 40-40:
undefined: cosmoscrypto
[failure] 40-40:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
GitHub Check: tests (00)
crypto/keys/multisig/multisig.go
[failure] 40-40:
undefined: cosmoscrypto
[failure] 40-40:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
GitHub Check: dependency-review
crypto/keys/multisig/multisig.go
[failure] 40-40:
undefined: cosmoscrypto
[failure] 123-123:
undefined: cosmoscrypto
Additional comments not posted (14)
store/go.mod (1)
13-13: Approved: Addition of new dependency.The addition of
github.com/cosmos/crypto v0.1.0aligns with the PR objective of decoupling fromcometbft/cryptoand usingcosmos/cryptoinstead.crypto/keyring/record.go (8)
Line range hint
25-30:
Approved: Function signature update.The function
newRecordhas been updated to usesdkcrypto.PubKey, which is consistent with the PR objective.
Line range hint
35-44:
Approved: Function signature update.The function
NewLocalRecordhas been updated to usesdkcrypto.PrivKeyandsdkcrypto.PubKey, which is consistent with the PR objective.
Line range hint
48-53:
Approved: Function signature update.The function
NewLedgerRecordhas been updated to usesdkcrypto.PubKey, which is consistent with the PR objective.
59-63: Approved: Function signature update.The function
NewOfflineRecordhas been updated to usesdkcrypto.PubKey, which is consistent with the PR objective.
66-70: Approved: Function signature update.The function
NewMultiRecordhas been updated to usesdkcrypto.PubKey, which is consistent with the PR objective.
73-77: Approved: Function signature update.The function
GetPubKeyhas been updated to usesdkcrypto.PubKey, which is consistent with the PR objective.
Line range hint
122-127:
Approved: Function signature update.The function
extractPrivKeyFromRecordhas been updated to usesdkcrypto.PrivKey, which is consistent with the PR objective.
131-138: Approved: Function signature update.The function
extractPrivKeyFromLocalhas been updated to usesdkcrypto.PrivKey, which is consistent with the PR objective.store/internal/maps/maps.go (2)
8-8: Approved: Update to SHA-256 hashing function.The change from
sha256.Sum256tosha256.Sumis consistent with the PR objective of usingcosmos/cryptoand may improve flexibility in handling the hash output.
39-43: Approved: Update to SHA-256 hashing function insetfunction.The change from
sha256.Sum256tosha256.Sumin thesetfunction is consistent with the PR objective of usingcosmos/cryptoand may improve flexibility in handling the hash output.crypto/keys/multisig/multisig.go (1)
109-114: LGTM!The changes are consistent with the objective of decoupling from
cometbft/crypto.simapp/go.mod (1)
52-52: LGTM!The addition of the dependency on
github.com/cosmos/crypto v0.1.1is consistent with the objective of decoupling fromcometbft/crypto.baseapp/baseapp.go (1)
703-703: LGTM! But verify the new hashing mechanism.The change from
tmhash.Sumtosha256.Sumis consistent with the objective of decoupling fromcometbft/crypto. Ensure that the new hashing mechanism works correctly and performs as expected.Verification successful
LGTM! But verify the new hashing mechanism.
The change from
tmhash.Sumtosha256.Sumis consistent with the objective of decoupling fromcometbft/crypto. The new hashing mechanism usingsha256.Sumis correctly applied and performs as expected across the codebase.
- baseapp/baseapp.go: Line 703 confirms the usage of
sha256.Sum.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new hashing mechanism using sha256.Sum works correctly. # Test: Search for the usage of the new hashing mechanism. Expect: Only occurrences of the new hashing mechanism. rg --type go -A 5 $'sha256.Sum'Length of output: 20916
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- crypto/keyring/record.go (7 hunks)
- crypto/keys/multisig/multisig.go (7 hunks)
- simapp/go.mod (2 hunks)
- x/auth/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- crypto/keyring/record.go
- crypto/keys/multisig/multisig.go
- simapp/go.mod
Additional comments not posted (2)
x/auth/go.mod (2)
20-20: Dependency removed:github.com/cosmos/crypto v0.1.2(indirect)The removal of
github.com/cosmos/crypto v0.1.2from the indirect dependencies is acknowledged.Ensure that there are no references to the indirect dependency.
20-20: Dependency added:github.com/cosmos/crypto v0.1.2The addition of
github.com/cosmos/crypto v0.1.2as a direct dependency is acknowledged.Ensure that this dependency is correctly utilized in the codebase.
Verification successful
Dependency added:
github.com/cosmos/crypto v0.1.2The addition of
github.com/cosmos/crypto v0.1.2as a direct dependency is acknowledged and verified to be correctly utilized in the codebase.
- Files with references:
x/auth/types/credentials.gox/auth/tx/builder.gox/auth/autocli.go- and many others across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new dependency `github.com/cosmos/crypto`. # Test: Search for imports of `cosmos/crypto`. Expect: Correct and consistent usage. rg --type go 'import.*cosmos/crypto'Length of output: 38
Script:
#!/bin/bash # Description: Verify any usage of the new dependency `github.com/cosmos/crypto`. # Test: Search for any references to `cosmos/crypto` in the codebase. rg 'cosmos/crypto'Length of output: 15829
|
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. |
Description
This PR contributes to solve the following issues:
Most imports made from
cometbft/cometbft/cryptowere removed and replaced with the (same) code in the new cosmos/cripto repo.Important: go.mod points to a commit instead to a release of such repo (TODO after this PR's review)
Changes made:
cometbft/cryptotocosmos/crypto.PubKeyandPrivKeyinterfaces design:proto.MessageLedgerPrivKeytype.sha256related ops are now imported fromcosmos/crypto/hash/sha256Randomops were moved tocosmos/crypto/randomcosmos/cryptoTODO (for subsequent PRs):
cosmos/crypto/curvescosmos-sdk/crypto/codecfrom cometbft's typesAuthor 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Dependencies
github.com/cosmos/cryptoto enhance cryptographic functionalities.Chores