improvements(cli): Enable specifying default key type for adding keys #53
Conversation
MalteHerrmann
requested review from
hanchon and
Vvaradinov
and removed request for
a team
WalkthroughThe changes introduced in this pull request include modifications to several files to enhance functionality and improve user interaction. The Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
server/util.go (1)
TODO for 'comet subcommands' is not tracked in any issues or PRs.
- Consider creating an issue to formally track the implementation of using comet subcommands and aliasing them to Tendermint.
🔗 Analysis chain
Line range hint
33-48: Improved command structure for Tendermint-related operations.The addition of a
tendermintCmdgroup enhances the organization of Tendermint-related subcommands, improving the overall CLI structure and user experience.Consider updating documentation to reflect the new command structure.
To ensure a smooth transition for users, it would be beneficial to update any relevant documentation or user guides to reflect this new command grouping.
Clarify the timeline for the TODO item.
The TODO comment suggests a future update to use comet subcommands. It would be helpful to clarify the timeline or priority for this change, especially if it might impact the current implementation.
To check for any existing issues or discussions related to this TODO, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related issues or discussions gh issue list --search "comet subcommands" gh pr list --search "comet subcommands"Length of output: 86
Script:
#!/bin/bash # Search for all occurrences of "comet subcommands" in the codebase, including comments rg --ignore-case "comet subcommands" # Alternatively, use ast-grep to find TODO comments related to "comet subcommands" ast-grep --lang go --pattern $'// TODO: *comet subcommands*$'Length of output: 207
client/keys.go (2)
22-28: LGTM! Consider creating an issue for the TODO.The added comments and function signature change align well with the PR objectives. They provide clear context for the new
defaultKeyTypeparameter and its recommended usage.Regarding the TODO comment about adjusting Tendermint mentions to CometBFT, would you like me to create a GitHub issue to track this task?
59-67: LGTM! Consider extracting the key type string to a constant.The implementation correctly sets the default key type based on the
defaultKeyTypeparameter. The error handling for theSetoperation is appropriate.For improved readability and maintainability, consider extracting the "eth_secp256k1" string to a named constant at the package level. This would make it easier to update if needed and reduce the risk of typos. For example:
const ( EthSecp256k1KeyType = "eth_secp256k1" ) // Then use it in the function: if defaultKeyType == EthSecp256k1KeyType { // ... }🧰 Tools
🪛 Gitleaks
60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- client/keys.go (2 hunks)
- client/keys/add.go (1 hunks)
- example_chain/local_node.sh (1 hunks)
- example_chain/osd/cmd/root.go (2 hunks)
- server/util.go (1 hunks)
🧰 Additional context used
🪛 Gitleaks
client/keys.go
60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
server/util.go (1)
Line range hint
33-48: Summary: Improved CLI structure with minimal impact on existing functionality.The changes introduced in this file effectively reorganize Tendermint-related commands into a more structured group. This improvement aligns well with the PR's objective of enhancing CLI functionality. The modification is focused and doesn't introduce any breaking changes to the existing codebase, making it a low-risk, high-value update.
client/keys/add.go (1)
100-100: Improved error message for key overwrite confirmation.This change enhances the error handling by providing a more informative error message when a user aborts the key overwrite confirmation. The new message includes the name of the key being overwritten, which improves clarity and assists in debugging or user interaction scenarios.
This improvement aligns well with the PR objectives of enhancing usability for developers. It provides clearer feedback during the key management process, particularly when dealing with existing keys.
example_chain/osd/cmd/root.go (1)
27-27: Importcosmoshdis correctly addedThe addition of the import statement for
cosmoshdis appropriate, as it is used to specify the default key typeSecp256k1Typelater in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
example_chain/README.md (1)
37-37: LGTM! Consider adding more details about remote debugging.The addition of the
--remote-debuggingflag is a valuable enhancement to the local node script. It provides developers with the option to build a binary suitable for remote debugging, which can be crucial for troubleshooting and development purposes.Consider adding a brief explanation or link to documentation about how to use this flag for remote debugging. This could help developers who are not familiar with the process.
example_chain/local_node.sh (2)
68-73: LGTM: Installation process updated for remote debuggingThe installation process has been correctly updated to use different build options when the
--remote-debuggingflag is set. This change enhances the debugging capabilities of the local node setup.Consider adding a comment explaining the purpose of the
nooptimization,nostripoptions for future maintainers:if [[ $BUILD_FOR_DEBUG == true ]]; then + # Disable optimizations and preserve debug symbols for remote debugging make install COSMOS_BUILD_OPTIONS=nooptimization,nostrip else make install fi
Line range hint
202-204: LGTM: Proposal periods adjusted for local testingThe modification of proposal periods in the genesis file is a good addition for local testing. It allows for faster proposal processes, which is beneficial in a development environment.
Consider adding a comment explaining why these values are changed and that they should not be used in production:
+# Adjust proposal periods for faster local testing. Do not use these values in production. sed -i.bak 's/"max_deposit_period": "172800s"/"max_deposit_period": "30s"/g' "$GENESIS" sed -i.bak 's/"voting_period": "172800s"/"voting_period": "30s"/g' "$GENESIS" sed -i.bak 's/"expedited_voting_period": "86400s"/"expedited_voting_period": "15s"/g' "$GENESIS"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- example_chain/README.md (1 hunks)
- example_chain/local_node.sh (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
example_chain/local_node.sh (3)
35-35: LGTM: New remote debugging flag implemented correctlyThe addition of the
BUILD_FOR_DEBUGvariable and the--remote-debuggingflag case in the argument parsing loop is well-implemented. This change aligns with the PR objective of improving debugging capabilities for the local node setup.Also applies to: 55-59
Line range hint
154-158: LGTM: Genesis file modifications for token denominationsThe added jq commands correctly modify the genesis file to set various token denominations to "aevmos". This ensures consistency across different modules (staking, governance, EVM, and minting) within the Evmos ecosystem.
Line range hint
1-256: Overall LGTM: Valuable improvements to local node setupThis update to the
local_node.shscript introduces several valuable improvements:
- Addition of a
--remote-debuggingflag for enhanced debugging capabilities.- Modification of the build process to support remote debugging when needed.
- Adjustment of genesis parameters, including token denominations and proposal periods, to better suit local testing environments.
These changes align well with the PR objectives and will significantly improve the developer experience when working with local Evmos nodes. The script maintains good practices in bash scripting and provides clear options for different use cases.
This PR adds the option to specify which key type to use by default when adding new keys using the
appd keys addcommand.Previously, this was defaulting to use
eth_secp256k1but this only applied to Evmos and other EVM-focussed chains. Now that we're integrating Cosmos-first chains too, we should open this up to selectsecp256k1by default if desired.Additionally, there was an error message improved and some TODOs added which I will be addressing on a follow up PR.
Again, additionally, a
--remote-debuggingflag is now accepted by thelocal_node.shscript to build and run a binary that can have a debugger attached to it via DAP.Summary by CodeRabbit
New Features
--remote-debuggingflag for improved script execution and debugging capabilities.Bug Fixes
Documentation