Add saving wallet config with bdk-cli wallet config

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.

[tvpeter]

Description

This PR adds bdk-cli wallet config command to save wallet configuration information to config.toml file in the data directory.

Fixes#192

Notes to the reviewers

• Reusing the exported serde crate from bdk_wallet did not offer the derive feature
• Fix out-of-bound error for shorten fn

Changelog notice

• Add wallet subcommand config to save wallet configs
• Add top-level wallets command to show all saved wallet configs

Checklists

Features

• command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config: wallet [-f] [-w <name>] config <wallet opts>
• All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing: wallet [-w <name>] sync | balance | new_address | etc...
• Repl uses the same wallet configs: repl [-w <name>]
• command to list all saved wallet configs: wallets
• throw warnings if using mainnet and saving a private descriptor in a config.

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[coveralls]

Pull Request Test Coverage Report for Build 21153320925

Details

• 99 of 395(25.06%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+2.8%) to 10.717%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils.rs	0	43	0.0%
src/config.rs	96	151	63.58%
src/handlers.rs	3	201	1.49%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	13.0%

Totals
Change from base Build 20714655583:	2.8%
Covered Lines:	269
Relevant Lines:	2510

---

💛 - Coveralls

[notmandatory]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

[tvpeter]

I like the approach for loading the config file parameters if the file exists, but is there a reason you didn't add a CLI bdk-cli wallet init ... command in rust to create the config file rather than using just ?

Creating the config file in rust should be easier to maintain/keep in sync than your just approach and shouldn't take too much more than adding the Serialize trait to WalletConfigInner. You should be able to reuse the WalletOpts to capture the values with clap that you need to put in the config file too.

Alright, I will update.

[tvpeter]

Alright, I will update.

@notmandatory I have updated the PR

[thunderbiscuit]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

[tvpeter]

I need to start keeping track of all these awesome new features I honestly see the PRs come in and can't keep up. This is super cool.

Thank you @thunderbiscuit

[notmandatory]

I spent some time today reviewing and even though this looks like a workable way to do it and is based on my suggestion I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

[tvpeter]

I'm afraid it's going to be a hassle to maintain. I've been experimenting with somehow using the different clap parsing functions (https://docs.rs/clap/latest/clap/trait.Parser.html) but so far haven't figured out a better way. I'd like to keep thinking about it. We might have to simplify the problem somehow such as by not trying to merge loaded and CLI args.

Thank you @notmandatory.
Yes, I agree that it will be challenging to maintain, as it involves two steps (pre clap parsing and after parsing) and even more difficult as they are in different locations in the codebase. I thought about moving everything to pre clap parsing based on the wallet sub-command the user has entered but that will clog the entry point as there will be lots of if...else checks.

Also, for the Parser trait, I read that it does not allow interacting with an external resource such as reading a file as I would want it.

I will also be trying out other approaches in case there is a cleaner way to handle it.

[notmandatory]

How about something like:

1. One command to save a wallet config, give wallet name or a default name is used, clap enforces required options; optional -f to override existing config:
   wallet [-f] [-w <name>] config <wallet opts>
2. All other wallet commands require that a config exists, use wallet name or the default is used. Give error if config is missing:
   wallet [-w <name>] sync | balance | new_address | etc...
3. Repl uses the same wallet configs:
   repl [-w <name>]
4. Add a command to list all available config names with summary of configured options:
   wallet list
5. Should throw warnings if using mainnet and saving a private descriptor in a config.

[notmandatory]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

[tvpeter]

I found an error when testing with testnet4, and a couple small nits should be fixed too. Otherwise this looks good.

Thank you for catching all these.
I have updated.

In the future if everyone prefers the config approach it should probably become the only way to use the wallet features.

Yes, I agree.

[thunderbiscuit]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

[tvpeter]

Thanks for this! I'll use it for sure once it gets in. I have two little things I want to ask, take them or leave them they're not blockers at all.

Question

When I run the example in the readme, I get the following config file:

network = "signet"
[wallets.my_wallet]
wallet = "my_wallet"network = "signet"ext_descriptor = "tr(tprv8Z.../0/*)#dtdqk3dx"int_descriptor = "tr(tprv8Z.../1/*)#ulgptya7"database_type = "sqlite"client_type = "electrum"server_url = "ssl://mempool.space:60602"batch_size = 10

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

Thank you for the review @thunderbiscuit
I will include this feedback in the next update as I also look at the configure_me lib to pick improvements.

[tvpeter]

as I also look at the configure_me lib to pick improvements

I have done a quick review of configure_me, and from my perspective, it may not be suitable for the CLI's use case. It uses a declarative approach to define the config file and generates a static file that updating it requires manually editing and recompiling. It is more suitable for an application-wide configuration and will be challenging to use for a per-wallet configuration. Also, I have not seen how it can handle conditional features. Lastly, I am not sure it can integrate with clap as they seemed to serve the same purpose (might be wrong). But there are improvements that we can pick from the library, things like validation and maybe how we merge fetched values into the CLI values.

[notmandatory]

thanks for looking into it, I agree we should stick with our simpler toml file approach.

[tvpeter]

The network is somehow added as a top-level key, but I suspect it's meant to only be added under the specific wallet config maybe? It doesn't prevent the cli from working as expected however so all is good there.

Thank you for noting this. It has been fixed now

Comment

I always find it hard when applications require a config file but it's empty by default. I have to search in their docs to find what's available, and even then it's hard to know if you miss anything. A nicer approach is when the config file comes pre-populated, with all keys commented out to start with but they're at least all there so I can use the config file to sort of "discover" what's available in the wallet. This is not always possible for all keys, but could be cool here. When doing this you could do it 2 ways:

1. Your config file could just come pre-made with a top-level commented section that has all keys available and the rest works as it currently does (easiest to implement). Note that you could also just add this as a codeblock in the readme (a complete list of available config options).
2. Each newly created wallet comes with its own section of complete config keys, some active and the rest commented out.

I have considered this approach and agree that having a configuration file for an app is beneficial. However, I’m not sure it applies to this tool for two reasons. Firstly, we do not expect users to manually edit the config file because we want clap to validate the passed values based on the features enabled by the user. Allowing users to modify this file could lead to errors when they run wallet commands.

Secondly, this configuration is intended specifically for the wallet and does not pertain to broader application settings. Users who are not testing their wallet flows may find the config file entirely irrelevant.

[notmandatory]

I pushed ceb9332 to update README examples to use new wallet config feature.

[notmandatory]

ACK ceb9332

Looks good! much easier to use.