Skip to content

Conversation

pranavkonde
Copy link
Contributor

Add Config Command for CLI Configuration Management

Overview

Added a comprehensive configuration management system to the RSK CLI, allowing users to customize their CLI experience through an interactive configuration command.

Features Added

Interactive Configuration Management

  • Network Settings: Set default network (mainnet/testnet)
  • Gas Settings: Configure default gas limit and gas price
  • API Key Management: Securely store and manage Alchemy API keys
  • Display Preferences: Toggle explorer links, gas details, block details, and compact mode
  • Wallet Preferences: Set auto-confirm transactions and default wallet

Technical Implementation

  • Configuration Persistence: Settings stored in rsk-cli-config.json
  • Type Safety: Full TypeScript support with proper interfaces
  • Validation: Input validation for all configuration options
  • Error Handling: Graceful error handling with fallback to defaults
  • Modular Design: Export functions for use in other commands

User Experience

  • Interactive Menu: User-friendly command-line interface
  • Current Settings Display: Clear overview of all current configurations
  • Reset Functionality: Easy reset to default settings
  • Secure Input: Password-style input for sensitive data (API keys)

Configuration Options

Category Setting Description
Network Default Network Choose between mainnet/testnet
Gas Default Gas Limit Set default gas limit (max 30M)
Gas Default Gas Price Set gas price in Gwei (0 for auto)
API Alchemy API Key Store API key for history command
Display Show Explorer Links Toggle transaction explorer links
Display Show Gas Details Toggle gas information display
Display Show Block Details Toggle block information display
Display Compact Mode Enable compact output format
Wallet Auto Confirm Skip password prompts for transactions
Wallet Default Wallet Set preferred wallet name

…ctive settings, gas/network defaults, API keys, display preferences, wallet options, and JSON persistence
Copy link
Collaborator

@chrisarevalo11 chrisarevalo11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pranavkonde

I liked the idea, and I have a few suggestions:

  • We need the proposed new configuration to be implemented in the commands.

  • Please remember not only to make the implementation but also to update the README, documenting these new options.

  • Also, please review the other comments I left in the PR.

Thanks!

@ezequiel-rodriguez
Copy link
Collaborator

hey @pranavkonde, the readme looks really good and also I checked that the cyan dividers are away :)

Let’s get this aligned so we can merge your PR and move forward with the reward — thanks!

We’ll need you to implement the use of the saved configurations in at least a few commands to make this contribution meaningful. Also, just a heads-up: we've been merging PRs from other contributors and adding support for MCP, so you'll need to resolve the conflicts as well.

We’ll definitely keep all of this in mind when it comes to your reward.
Thanks again!

@pranavkonde
Copy link
Contributor Author

hey @pranavkonde, the readme looks really good and also I checked that the cyan dividers are away :)

Let’s get this aligned so we can merge your PR and move forward with the reward — thanks!

We’ll need you to implement the use of the saved configurations in at least a few commands to make this contribution meaningful. Also, just a heads-up: we've been merging PRs from other contributors and adding support for MCP, so you'll need to resolve the conflicts as well.

We’ll definitely keep all of this in mind when it comes to your reward. Thanks again!

Hi @ezequiel-rodriguez

I’ve made the suggested changes. Everything should now be in sync and ready for review.

Fixing visible sensitive information
Just for security purposes not showing any part of the api key
@scguaquetam
Copy link
Collaborator

Hello @pranavkonde , I did a small change to protect the alchemy api key (requested by codeql)
and also , please take in account the comment from @ezequiel-rodriguez in his last comment
We’ll need you to implement the use of the saved configurations in at least a few commands to make this contribution meaningful..
That means the possible ussage of this new function you are adding into other modules of the CLI.
Let us know if you have any questions

@pranavkonde
Copy link
Contributor Author

pranavkonde commented Aug 28, 2025

Hello @pranavkonde , I did a small change to protect the alchemy api key (requested by codeql) and also , please take in account the comment from @ezequiel-rodriguez in his last comment We’ll need you to implement the use of the saved configurations in at least a few commands to make this contribution meaningful.. That means the possible ussage of this new function you are adding into other modules of the CLI. Let us know if you have any questions

Hello @scguaquetam
I've integrated the configuration system across the main commands (transaction, deploy, bridge, balance, verify).

@scguaquetam
Copy link
Collaborator

Hello @pranavkonde , thanks for the changes.
I am testing the new feature right now, but I see an error on the execution.
When I configure the network to TESTNET
image
and then check again the status
image
it shows it is the setting on testnet
but, when I use the balance module, it takes the balance from mainnet instead of testnet as it is supposed to be configured
image
It is ignoring the configuration set on the config module, and instead is taking the --testnet tag only.

Additionally, when I tested the deploy command, it shows an error when trying to deploy a contract, looks like your implementation is breaking the function, because it does not work with and without the --testnet tag
image
Please take a look and test your implementation, I am testing deploying the same contract on the main (current) branch and it works correctly.
IMPORTANT:
Since we’re releasing a new version of the Hacktivator program, I need to ask you to have these changes ready by the weekend. Otherwise, we’ll need to send you a smaller reward than originally planned and take care of it on our side next week. I know it’s a bit of an unusual situation, but it’s really for the benefit of the whole community. Thanks for your understanding

@pranavkonde
Copy link
Contributor Author

Hello @pranavkonde , thanks for the changes. I am testing the new feature right now, but I see an error on the execution. When I configure the network to TESTNET image and then check again the status image it shows it is the setting on testnet but, when I use the balance module, it takes the balance from mainnet instead of testnet as it is supposed to be configured image It is ignoring the configuration set on the config module, and instead is taking the --testnet tag only.

Additionally, when I tested the deploy command, it shows an error when trying to deploy a contract, looks like your implementation is breaking the function, because it does not work with and without the --testnet tag image Please take a look and test your implementation, I am testing deploying the same contract on the main (current) branch and it works correctly. IMPORTANT: Since we’re releasing a new version of the Hacktivator program, I need to ask you to have these changes ready by the weekend. Otherwise, we’ll need to send you a smaller reward than originally planned and take care of it on our side next week. I know it’s a bit of an unusual situation, but it’s really for the benefit of the whole community. Thanks for your understanding

Thank you for catching these issues! I've made the following fixes

@scguaquetam
Copy link
Collaborator

Hello @pranavkonde I am a little bit confused here
image
In deploy.ts you changed const isTestnet = params.testnet ?? (config.defaultNetwork === 'testnet');for const isTestnet = params.testnet !== undefined ? params.testnet : (config.defaultNetwork === 'testnet');, apparently that is the fix for the error on deploy command , nevertheless you are using the same const isTestnet = params.testnet ?? (config.defaultNetwork === 'testnet'); for :

  • balance.ts
  • bridge.ts
  • transaction.ts
  • verify.ts

Doesn't should all the implementations have the same fix?
Can you confirm to me if you tested each one of this configurations and worked well? I think they are not working properly.

@pranavkonde
Copy link
Contributor Author

In deploy.ts you changed const isTestnet = params.testnet ?? (config.defaultNetwork === 'testnet');for const isTestnet = params.testnet !== undefined ? params.testnet : (config.defaultNetwork === 'testnet');, apparently that is the fix for the error on deploy command , nevertheless you are using the same const isTestnet = params.testnet ?? (config.defaultNetwork === 'testnet'); for :

  • balance.ts
  • bridge.ts
  • transaction.ts
  • verify.ts

Doesn't should all the implementations have the same fix? Can you confirm to me if you tested each one of this configurations and worked well? I think they are not working properly.

Hello @scguaquetam
I have standardized the testnet configuration handling across all command files. Could you please take a look?

@scguaquetam
Copy link
Collaborator

scguaquetam commented Sep 22, 2025

I see this contribution correct, I don't see errors, just a question I have on the transaction module, which @pranavkonde created previously (If you @pranavkonde can fix it and configure transaction module you created before to what we have today and connect with the index.ts file, that would mean more price to this contribution, otherwise if you want to leave it as it is, you could let the contribution until this point). Other than that it is okay for merging.

@pranavkonde
Copy link
Contributor Author

I see this contribution correct, I don't see errors, just a question I have on the transaction module, which @pranavkonde created previously (If you @pranavkonde can fix it and configure transaction module you created before to what we have today and connect with the index.ts file, that would mean more price to this contribution, otherwise if you want to leave it as it is, you could let the contribution until this point). Other than that it is okay for merging.

Thanks for the review! I’ve made the requested changes and updated the transaction module accordingly, connecting it with the index.ts file.

@pranavkonde
Copy link
Contributor Author

@scguaquetam Thank you for catching these issues! I've made the following fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants