chore: sync with upstream #183
Conversation
…) (cosmos#21840) Co-authored-by: Alessio Treglia <al@essio.dev> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
…osmos#21861) Co-authored-by: Akhil Kumar P <36399231+akhilkumarpilli@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
WalkthroughThe pull request introduces a new Linux-only backend in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 🔇 Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
crypto/keyring/keyring_linux_test.go (1)
17-51: LGTM with suggestions: Well-structured test function with room for expansion.The
TestNewKeyctlKeyringfunction is well-structured using a table-driven approach, which is excellent for maintainability and future expansion. The use oft.TempDir()for temporary directories is a good practice.Suggestions for improvement:
- Consider adding more test cases to cover different scenarios (e.g., error cases, different backends).
- It might be beneficial to add assertions on the properties of the created keyring object when successful.
- Consider adding a comment explaining the purpose of the
getCodec()function for clarity.Here's an example of how you could expand the test cases:
tests := []struct { name string appName string backend string dir string userInput io.Reader cdc codec.Codec expectedErr error }{ { name: "keyctl backend success", appName: "cosmos", backend: BackendKeyctl, dir: t.TempDir(), userInput: strings.NewReader(""), cdc: cdc, expectedErr: nil, }, { name: "invalid backend", appName: "cosmos", backend: "invalid", dir: t.TempDir(), userInput: strings.NewReader(""), cdc: cdc, expectedErr: errors.New("unknown backend"), }, // Add more test cases here }And for successful cases, you could add assertions on the keyring properties:
if tt.expectedErr == nil { require.NoError(t, err) require.NotNil(t, kr) // Add more specific assertions here, e.g.: // require.Equal(t, tt.appName, kr.AppName()) }crypto/keyring/keyring_other.go (1)
14-29: LGTM: Options struct is well-defined, with a minor suggestion.The
Optionsstruct is well-designed and includes clear comments for each field. The use of function types for Ledger operations allows for flexibility in implementation.Consider adding a brief comment above the
Optionsstruct to describe its overall purpose and usage within the package. This would enhance the documentation and make it easier for other developers to understand how to use this struct.+// Options defines the configuration for the keyring, including supported algorithms +// and Ledger-specific settings. It is used to customize the behavior of the keyring. type Options struct { // ... existing fields ... }crypto/keyring/doc.go (2)
36-37: Improve consistency and provide additional information for thekeyctlbackend.The new
keyctlbackend description is informative and concise. However, to maintain consistency with other backend descriptions and provide more value to users, consider the following suggestions:
- Add a link to more information about the Linux kernel's security key management system.
- Adjust the indentation to match other entries (add one space at the beginning of line 37).
Here's a suggested revision:
-// keyctl This backend leverages the Linux's kernel security key management system -// to store cryptographic keys securely in memory. This is available on Linux only. +// keyctl This backend leverages the Linux kernel's security key management system +// to store cryptographic keys securely in memory. This is available on Linux only: +// https://man7.org/linux/man-pages/man7/keyrings.7.htmlThis change improves consistency with other backend descriptions and provides users with a reference for further information.
Line range hint
1-41: LGTM with a minor suggestion for improvement.The addition of the
keyctlbackend description is appropriate and aligns well with the existing documentation. The change effectively communicates the purpose and limitation of the new backend option.To further enhance the documentation:
- Consider adding a brief mention of the
keyctlbackend in the introductory paragraph of the "The backends:" section. This would provide a complete overview of all available backends at the beginning of the list.Here's a suggested addition to the introductory paragraph (around line 22):
// The backends: +// +// The following backends are available for key storage:This small addition would improve the overall structure and readability of the backend descriptions.
crypto/keyring/keyring.go (2)
Line range hint
162-203: LGTM with a minor suggestionThe renaming of
NewtonewKeyringGenericand making it unexported is a good change, as it allows for more flexibility in the package's API. The function's logic remains unchanged, which is good for maintaining consistency.However, consider adding a comment explaining why this function is now unexported and how it should be used internally.
Consider adding a comment above the function to explain its purpose and usage:
// newKeyringGeneric creates a new instance of a keyring based on the specified backend. // This function is unexported and should only be used internally by the keyring package. func newKeyringGeneric( appName, backend, rootDir string, userInput io.Reader, cdc codec.Codec, opts ...Option, ) (Keyring, error) { // ... (existing function body) }
Line range hint
1-1: Summary of changes and required follow-up actionsThe main changes in this file involve removing the
Optionsstruct and renaming theNewfunction tonewKeyringGeneric. While these changes appear to be part of a refactoring effort to improve the API, there are some inconsistencies and potential impacts that need to be addressed:
- Ensure that all references to the old
Newfunction have been updated throughout the codebase.- Verify that the removal of the
Optionsstruct is consistent and that all related code has been updated accordingly.- Consider adding explanatory comments for the newly unexported
newKeyringGenericfunction.- Review the results of the verification scripts and make any necessary adjustments.
Please address these points to ensure the refactoring is complete and consistent across the entire codebase.
CHANGELOG.md (2)
Line range hint
70-74: Important: Client Breaking ChangesThis section outlines several changes that will impact existing client implementations:
- Removal of telemetry for counting votes and proposals in x/gov module.
- Changes in event handling in ABCI, including the removal of duplicate events and addition of a msg_index to all event attributes.
- Use of the same port for gRPC-Web and the API server.
These changes are likely to require updates to client-side code, monitoring systems, and potentially deployment configurations. Users should carefully review these changes and update their implementations accordingly when upgrading to v0.50.2.
Consider providing migration guides or examples to help users adapt to these breaking changes, especially for the event handling modifications and the gRPC-Web port change.
🧰 Tools
Markdownlint
47-47: null
Multiple headings with the same content(MD024, no-duplicate-heading)
Line range hint
76-89: CLI Breaking Changes: Update Your Commands and ScriptsThis section outlines several important changes to the CLI that will require updates to existing scripts and workflows:
- Migration to AutoCLI has led to changes in CLI outputs and pagination flag names.
- The
statuscommand now returns snake case JSON keys instead of pascal case.- Removal of the
iavl-lazy-loadingconfiguration.- Changes in the x/gov module's proposal status flags and values.
- Split of some bank commands and changes in their naming.
These changes will require updates to any scripts, documentation, or processes that interact with the CLI. Users should carefully review their CLI usage and update accordingly when upgrading to v0.50.2.
To ease the transition:
- Consider providing a migration script that can automatically update common CLI commands in scripts.
- Update all relevant documentation to reflect these CLI changes.
- Clearly communicate these changes to users, possibly with a dedicated upgrade guide section for CLI changes.
🧰 Tools
Markdownlint
47-47: null
Multiple headings with the same content(MD024, no-duplicate-heading)
crypto/keyring/keyring_linux.go (3)
20-24: Add documentation comments for exported functionsThe exported functions
KeyctlScopeUser,KeyctlScopeUserSession,KeyctlScopeSession,KeyctlScopeProcess, andKeyctlScopeThreadlack documentation comments. According to Go conventions, exported functions should have comments that begin with the function name to improve code readability and comply withgolintrecommendations.
45-59: Consider omitting unused parameters if not requiredThe function
newKeyctlBackendConfigincludes unused parameters named_ string,_ io.Reader. If this function doesn't need to match a predefined interface, consider removing these unused parameters to simplify the function signature.
71-76: Consider renaming variabledbto clarify its purposeIn the
Newfunction, the variabledbholds the keyring instance returned bykeyring.Open. To improve code readability, consider renaming it tokrorkeyringInstanceto better reflect its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (3)
simapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/staking/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- api/cosmos/staking/v1beta1/query.pulsar.go (1 hunks)
- crypto/keyring/doc.go (1 hunks)
- crypto/keyring/keyring.go (1 hunks)
- crypto/keyring/keyring_linux.go (1 hunks)
- crypto/keyring/keyring_linux_test.go (1 hunks)
- crypto/keyring/keyring_other.go (1 hunks)
- proto/cosmos/staking/v1beta1/query.proto (1 hunks)
- simapp/go.mod (1 hunks)
- tests/go.mod (1 hunks)
- x/staking/autocli.go (1 hunks)
🔇 Additional comments not posted (15)
crypto/keyring/keyring_linux_test.go (2)
1-4: LGTM: Correct file structure and build tags.The file structure is well-organized with appropriate build tags for Linux-specific tests. The package declaration is correct.
6-15: LGTM: Appropriate imports for testing.The import statements include all necessary packages for testing, including the testify/require package for concise assertions and the cosmos-sdk codec. This suggests a well-structured test setup.
crypto/keyring/keyring_other.go (3)
1-4: LGTM: Build tags and package declaration are correct.The build tags are properly formatted and placed at the top of the file, ensuring this code is only compiled for non-Linux environments. The package name
keyringis appropriate for the functionality it provides.
6-12: LGTM: Import statements are appropriate.The import statements are relevant to the functionality provided by this package. There are no unused imports, which is good for code cleanliness.
31-35: LGTM: New function is well-defined, with a request for clarification.The
Newfunction is well-structured and provides a flexible way to create a new Keyring instance. The use of variadic options is a good practice for extensibility.Could you please provide more information about the
newKeyringGenericfunction? It's not visible in this file, and understanding its implementation would be helpful for a complete review. Additionally, let's verify if this function is defined in another file:x/staking/autocli.go (1)
116-116: Verify the impact of makingdst_validator_addroptionalThe change to make
dst_validator_addroptional in theRedelegationsRPC method is significant. While it provides more flexibility in command usage, it raises some concerns:
- Ensure that the backend logic can handle cases where
dst_validator_addris not provided.- Update the command's documentation and user guides to reflect this change.
- Consider adding a note in the command's
Longdescription to explain the optional nature ofdst_validator_addr.To ensure consistency and proper handling, run the following script:
Consider updating the
Usefield to reflect the optional nature ofdst_validator_addr, for example:Use: "redelegation [delegator-addr] [src-validator-addr] [dst-validator-addr]",to
Use: "redelegation [delegator-addr] [src-validator-addr] [dst-validator-addr (optional)]",tests/go.mod (1)
41-41: LGTM: Cosmos SDK client library updated to latest beta version.The update of
cosmossdk.io/client/v2tov2.0.0-beta.5aligns with the PR objective of syncing with upstream changes. This change should improve the test environment by using the latest beta version of the Cosmos SDK client library.To ensure a smooth transition, please verify the following:
- Check the changelog or release notes for
cosmossdk.io/client/v2 v2.0.0-beta.5to identify any breaking changes or new features that might affect the tests.- Review if any test updates are needed due to this change.
You can use the following command to check for any breaking changes or significant updates:
If there are any breaking changes or significant updates, please ensure that the necessary adjustments are made in the test suite.
simapp/go.mod (1)
9-9: Approve the upgrade, but verify compatibility.The upgrade from
v2.0.0-beta.4.0.20240918122632-1879050ca719tov2.0.0-beta.5ofcosmossdk.io/client/v2is a positive change, moving from a specific commit to a tagged beta version. This improves dependency management and tracking.However, as this is still a beta version, please ensure:
- The upgrade doesn't introduce any breaking changes.
- All functionalities using this client library are still working as expected.
Run the following script to check for potential breaking changes or deprecated features:
Review the output carefully for any usage that might be affected by the version change. If any potential issues are found, please update the corresponding code accordingly.
proto/cosmos/staking/v1beta1/query.proto (1)
295-298: Approve the refinement of validator address types.The change from
cosmos.AddressStringtocosmos.ValidatorAddressStringforsrc_validator_addranddst_validator_addrfields in theQueryRedelegationsRequestmessage is a good refinement. It provides more specific typing for validator addresses, which can improve type safety and clarity in the codebase.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in the use of address types for validators across the project.
✅ Verification successful
Verified the refinement of validator address types.
All instances of
cosmos.AddressStringfor validator addresses have been successfully updated tocosmos.ValidatorAddressStringacross the codebase. This ensures improved type safety and clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of cosmos.AddressString used for validator addresses # and verify that cosmos.ValidatorAddressString is used consistently for validator addresses. # Search for remaining instances of cosmos.AddressString used for validator addresses echo "Checking for remaining instances of cosmos.AddressString used for validator addresses:" rg --type proto 'validator.*addr.*cosmos.AddressString' # Verify consistent usage of cosmos.ValidatorAddressString echo "Verifying consistent usage of cosmos.ValidatorAddressString for validator addresses:" rg --type proto 'validator.*addr.*cosmos.ValidatorAddressString'Length of output: 546
Script:
#!/bin/bash # Description: Check for any remaining instances of cosmos.AddressString used for validator addresses # and verify that cosmos.ValidatorAddressString is used consistently for validator addresses in .proto files. # Search for remaining instances of cosmos.AddressString used for validator addresses in .proto files echo "Checking for remaining instances of cosmos.AddressString used for validator addresses:" rg 'validator.*addr.*cosmos\.AddressString' -g "*.proto" # Verify consistent usage of cosmos.ValidatorAddressString for validator addresses in .proto files echo "Verifying consistent usage of cosmos.ValidatorAddressString for validator addresses:" rg 'validator.*addr.*cosmos\.ValidatorAddressString' -g "*.proto"Length of output: 5799
crypto/keyring/keyring.go (2)
Line range hint
1-1: Verify the consistency and impact of removing theOptionsstructThe
Optionsstruct has been removed, but there are still references to it in the code (e.g., in thenewKeystorefunction). This inconsistency needs to be addressed.Let's check for any remaining usage of the
Optionsstruct in this file and the rest of the codebase:#!/bin/bash # Search for any remaining usage of the 'Options' struct in this file rg --type go 'type\s+Options\s+struct' crypto/keyring/keyring.go # Search for any remaining usage of the 'Options' struct in other files rg --type go 'Options\s+struct' -g '!crypto/keyring/keyring.go' # Search for any remaining usage of the 'Options' type rg --type go '\boptions\s+Options\b' -g '!crypto/keyring/keyring.go'Please review the results of these searches and ensure that all references to the
Optionsstruct have been properly updated or removed.
Line range hint
162-203: Verify the impact of renamingNewtonewKeyringGenericThe function
Newhas been renamed tonewKeyringGeneric. This change might affect other parts of the codebase that were using theNewfunction to create a keyring instance.To ensure this change doesn't break existing functionality, let's check for any remaining usage of the old
Newfunction:Also, verify that
newKeyringGenericis being called appropriately in place of the oldNewfunction:✅ Verification successful
Renaming of
NewtonewKeyringGenericVerifiedAll instances of the old
Newfunction have been successfully updated tonewKeyringGeneric. No remaining usages ofNewwere found outsidecrypto/keyring/keyring.go.
- Updated files:
crypto/keyring/keyring_linux.gocrypto/keyring/keyring_other.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the old 'New' function rg --type go 'New\s*\(\s*appName\s*,\s*backend\s*,\s*rootDir\s*,\s*userInput\s*,\s*cdc\s*,.*\)' -g '!crypto/keyring/keyring.go'Length of output: 2404
Script:
#!/bin/bash # Search for usage of the new 'newKeyringGeneric' function rg --type go 'newKeyringGeneric\s*\(\s*appName\s*,\s*backend\s*,\s*rootDir\s*,\s*userInput\s*,\s*cdc\s*,.*\)'Length of output: 332
CHANGELOG.md (3)
41-45: Features section looks good!The new features are clearly described and provide valuable additions to the SDK. The debug commands for application codec types, the ability to use base64 encoded public keys, and the shutdown grace flag are all useful improvements.
Line range hint
47-57: Improvements section is comprehensive and well-documented.The improvements cover a wide range of areas in the SDK, enhancing functionality and addressing various issues. All changes are clearly explained.
One point to note:
- The change in line 51 (server configuration for disabling colored logs) might affect existing setups that rely on colored logs. Users should be aware of this new flag when upgrading.
Overall, these improvements contribute positively to the SDK's functionality and user experience.
🧰 Tools
Markdownlint
47-47: null
Multiple headings with the same content(MD024, no-duplicate-heading)
Line range hint
59-68: Bug fixes section addresses important issues.The bug fixes cover a range of components and modules, improving the overall stability and correctness of the SDK. All fixes are clearly explained.
Special attention should be given to:
- The fix in line 65 (baseapp) changes the behavior of post handlers, which now run on non-successful transaction executions too. This could potentially impact existing implementations that assume post handlers only run on successful transactions.
Overall, these bug fixes contribute to a more robust and reliable SDK.
🧰 Tools
Markdownlint
47-47: null
Multiple headings with the same content(MD024, no-duplicate-heading)
crypto/keyring/keyring_linux.go (1)
1-2: LGTMThe build constraints are correctly specified for Linux-only builds.
Description
Closes: #XXXX
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Release Notes
New Features
keyctl.Bug Fixes
Documentation
keyctlbackend.Dependency Updates
cosmossdk.io/client/v2to versionv2.0.0-beta.5.CLI Improvements
dst_validator_addrfrom mandatory to optional in the command-line interface.