Skip to content

feat(use-clap-for-cli): use clap to parse CLI-Args #2215#2510

Merged
onur-ozkan merged 4 commits intoGLEECBTC:devfrom
BigFish2086:2215-using-clap-for-cli-args
Jul 10, 2025
Merged

feat(use-clap-for-cli): use clap to parse CLI-Args #2215#2510
onur-ozkan merged 4 commits intoGLEECBTC:devfrom
BigFish2086:2215-using-clap-for-cli-args

Conversation

@BigFish2086
Copy link
Copy Markdown

@BigFish2086 BigFish2086 commented Jun 28, 2025

use clap to parse CLI-Args as described in here #2215

- had to set clap version to "=v4.2" in Cargo.toml, because cargo was trying to fetch clap version 4.5 and it requires a higher version of rust tool-chain (1.74) and the current one is (1.72)
- clap provides both "-h" or "--help" for the help
- override the behaviour of the help to be able to show the same message as before with the version of KDF_VERSION provided by the git-commit
- modes & subcommands like `btc2kmd`, `events`, `vanity`, `update_config` weren't actually used or handled by the code, so I removed them
@BigFish2086 BigFish2086 changed the title feat(use-clap-for-kdf): use clap to parse CLI-Args #2215 feat(use-clap-for-cli): use clap to parse CLI-Args #2215 Jun 28, 2025
Cargo.toml Outdated
chrono = "0.4.23"
cfg-if = "1.0"
clap = { version = "4.2", features = ["derive"] }
clap = { version = "=4.2", features = ["derive"] }
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cargo was trying to get clap version 4.5, so I had to set it to v4.2 since v4.5 requires a higher rust tool-chain version

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can remove = part as lockfile is now created.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated in 7074d52

"#;

#[derive(Parser, Debug)]
#[command(about="Komodo DeFi Framework Daemon", long_about=None, after_help=EXTRA_HELP_MESSAGE)]
Copy link
Copy Markdown
Author

@BigFish2086 BigFish2086 Jun 28, 2025

Choose a reason for hiding this comment

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

I used after_help here to show the extra help message as it was before, but I suggest that we can use a flag like
-hh so the user can control when to show these extra help messages

.collect();
let mut args: Vec<*const c_char> = args.iter().map(|s| s.as_ptr() as *const c_char).collect();
args.push(null());

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

wasn't really sure if that part still relevant, It might have been there for that "C version of the main method" but I couldn't find it

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That was migrated long time ago and it's not relevant anymore.

println!("{}: {}", Cli::command().get_about().unwrap(), version);
return;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was trying to keep the version command showing the git commit or KDF_VERSION as before, but couldn't do it through the derived version without duplicating this line from mm2src/mm2_bin_lib/src/mm2_bin.rs

#[cfg(not(target_arch = "wasm32"))]
const KDF_VERSION: &str = env!("KDF_VERSION");

so i went for this approach instead

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It was println!("Komodo DeFi Framework: {version}"); before and version is already identical to KDF_VERSION.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated in 726791a

vanity {substring} .. Tries to find an address with the given substring.
update_config {SRC} {DST} .. Update the configuration of coins from the SRC config and save it to DST file.
{JSON configuration} .. Run the MarketMaker daemon.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

these modes or sub commands weren't handled any were except for passing a JSON configuration

@mariocynicys mariocynicys requested a review from Copilot June 28, 2025 14:33
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CLI argument parsing in the daemon by replacing the manual parsing logic with clap functionality.

  • Replaces old manual command-line argument parsing (including help/version handling) with clap's derive-based parsing.
  • Updates the get_mm2config function to accept a JSON configuration passed from clap.
  • Adjusts Cargo.toml dependencies to reflect the new usage of clap and reorganizes cfg-if.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
mm2src/mm2_main/src/mm2.rs Migrates CLI parsing to clap and removes the legacy argument handling.
mm2src/mm2_main/Cargo.toml Reorders dependency declarations and adds clap.workspace.
Cargo.toml Pins the clap version to 4.2 explicitly.
Comments suppressed due to low confidence (2)

Cargo.toml:79

  • Pinning the clap version ensures consistency but may limit future upgrades; please confirm this design decision aligns with the project's dependency management strategy.
clap = { version = "=4.2", features = ["derive"] }

mm2src/mm2_main/src/mm2.rs:263

  • Ensure that the new CLI handling with clap fully replicates the behavior of the previous manual parsing, particularly regarding help message handling on platforms (e.g., Windows '/?') that were explicitly supported.
    let cli = Cli::parse();

Copy link
Copy Markdown

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup, very appreciated!

const CUSTOM_PAYMENT_LOCKTIME_DEFAULT: u64 = 900;

const EXTRA_HELP_MESSAGE: &str = r#"
Some (but not all) of the JSON configuration parameters (* - required):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we shouldn't have a reference for the JSON parameters as then can quickly become obsolete and outdated.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated in 2d62cee

.collect();
let mut args: Vec<*const c_char> = args.iter().map(|s| s.as_ptr() as *const c_char).collect();
args.push(null());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That was migrated long time ago and it's not relevant anymore.

println!("{}: {}", Cli::command().get_about().unwrap(), version);
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It was println!("Komodo DeFi Framework: {version}"); before and version is already identical to KDF_VERSION.

help();
let cli = Cli::parse();
if cli.version {
println!("{}: {}", Cli::command().get_about().unwrap(), version);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it's better to hard-code the version output prefix like before:

Suggested change
println!("{}: {}", Cli::command().get_about().unwrap(), version);
println!("Komodo DeFi Framework: {version}");

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated in 726791a

Cargo.toml Outdated
chrono = "0.4.23"
cfg-if = "1.0"
clap = { version = "4.2", features = ["derive"] }
clap = { version = "=4.2", features = ["derive"] }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can remove = part as lockfile is now created.

@onur-ozkan onur-ozkan merged commit eab8081 into GLEECBTC:dev Jul 10, 2025
1 check failed
dimxy pushed a commit that referenced this pull request Jul 13, 2025
* dev:
  fix(eth): Correctly implement ETH max withdrawal logic (#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510)
  feat(orderbook): expirable maker orders (#2516)
  improvement(eth): drop parity support (#2527)
  chore(release): finalize changelog for v2.5.0-beta (#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (#2514)
  fix(walletconnect): centralize connection and retry logic (#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (#2504)
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Jul 16, 2025
* refactor-gas-fee-policy: (30 commits)
  revert change in test_enable_custom_erc20
  fix(test) disable get_swap_gas_fee_policy for tests (no platform coin in tests)
  revert tokio test and use block_on for eth docker test
  Refactor gas fee code: get max_eth_tx_type from platform coin, add gas fee adjust coin params, rename "set_swap_transaction_fee_policy" rpc to "set_swap_gas_fee_policy", force set_swap_gas_fee_policy to store policy in the platform coin
  fix(eth): Propagate structured EIP-1559 fee errors (GLEECBTC#2532)
  fix(eth): Correctly implement ETH max withdrawal logic (GLEECBTC#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args GLEECBTC#2215 (GLEECBTC#2510)
  feat(orderbook): expirable maker orders (GLEECBTC#2516)
  improvement(eth): drop parity support (GLEECBTC#2527)
  chore(release): finalize changelog for v2.5.0-beta (GLEECBTC#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (GLEECBTC#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (GLEECBTC#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (GLEECBTC#2514)
  fix(walletconnect): centralize connection and retry logic (GLEECBTC#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (GLEECBTC#2504)
  chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473)
  chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443)
  feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497)
  chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494)
  chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492)
  ...

# Conflicts:
#	mm2src/coins/eth.rs
#	mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs
#	mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs
#	mm2src/coins/eth/eth_utils.rs
#	mm2src/coins/eth/eth_withdraw.rs
#	mm2src/coins/eth/v2_activation.rs
#	mm2src/coins/lp_coins.rs
#	mm2src/coins/qrc20.rs
#	mm2src/coins/qrc20/swap.rs
#	mm2src/coins_activation/src/platform_coin_with_tokens.rs
#	mm2src/coins_activation/src/token.rs
#	mm2src/mm2_main/src/lp_swap/check_balance.rs
#	mm2src/mm2_main/src/lp_swap/maker_swap.rs
#	mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs
#	mm2src/mm2_main/src/rpc/lp_commands/ext_api/ext_api_types.rs
#	mm2src/mm2_main/src/rpc/lp_commands/mod.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/errors.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs
#	mm2src/mm2_main/src/rpc/lp_commands/tokens.rs
#	mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
#	mm2src/trading_api/src/one_inch_api/classic_swap_types.rs
#	mm2src/trading_api/src/one_inch_api/client.rs
#	mm2src/trading_api/src/one_inch_api/portfolio_types.rs
dimxy pushed a commit that referenced this pull request Aug 1, 2025
* dev: (21 commits)
  feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499)
  feat(utxo): add new fixed txfee option for DINGO-like coins (#2454)
  ci(pull-requests): review notification bot (#2468)
  improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538)
  bless clippy (#2560)
  refactor(toolchain): use latest available stable compiler (#2557)
  feat(wallet): implement unified offline private key export API (#2542)
  improve note for docker test start failure (#2550)
  fix(DOCS): add note for macos to fix docker containers startup failure (#2544)
  refactor(toolchain): general stabilization for stable rust (#2528)
  fix(ci): adds nodejs 20 to ci-container (#2536)
  fix(WASM and Debian): fix build failures (#2534)
  improvement(event-streaming): impl DeriveStreamerId trait for all streamers (#2489)
  fix(eth): Propagate structured EIP-1559 fee errors (#2532)
  fix(eth): Correctly implement ETH max withdrawal logic (#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510)
  feat(orderbook): expirable maker orders (#2516)
  improvement(eth): drop parity support (#2527)
  chore(release): finalize changelog for v2.5.0-beta (#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444)
  ...

# Conflicts:
#	mm2src/coins/lp_coins.rs
#	mm2src/coins/rpc_command/get_new_address.rs
#	mm2src/trezor/src/eth/eth_command.rs
dimxy pushed a commit that referenced this pull request Oct 15, 2025
* feat(use-clap-for-kdf): use clap to parse CLI-Args #2215

- had to set clap version to "=v4.2" in Cargo.toml, because cargo was trying to fetch clap version 4.5 and it requires a higher version of rust tool-chain (1.74) and the current one is (1.72)
- clap provides both "-h" or "--help" for the help
- override the behaviour of the help to be able to show the same message as before with the version of KDF_VERSION provided by the git-commit
- modes & subcommands like `btc2kmd`, `events`, `vanity`, `update_config` weren't actually used or handled by the code, so I removed them

* fix(use-clap-for-cli): remove '=' from Cargo.toml as the Cargo.lock file is already created.

* fix(use-clap-for-cli): hard-code the version output prefix to keep it like before

* fix(use-clap-for-cli): remove reference for the JSON parameters from help as they can quickly become obsolete and outdated.
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.

3 participants