Add command to delete saved wallet config

[yan-pi]

Description

Adds wallets delete <wallet_name> to remove a saved wallet configuration.
Saved configurations can be listed with wallets list.

Resolves #310.

Notes to the reviewers

List and delete use separate handlers and outputs.

Delete only removes the entry from config.toml.
Wallet database files arenot removed.

Changelog notice

• Added wallets delete to remove a saved wallet configuration

Checklists

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

[tvpeter]

Only the config.toml entry is affected, wallet data on disk is not touched (dont know if its the correct path).

So wallet config only writes config.toml. The DB file is created lazily during an actual wallet operation. So deleting a config that was never used leaves nothing on disk. However, if the wallet was used, the DB file remains.

[tvpeter]

I agree with the approach of the delete operation but the returned type of WalletList is unnecesary. If returning a message is not possible, then it is better to split the wallets into something like wallets list and wallets --delete <name>. So WalletsSubcommand will be the top-level.

[tvpeter]

Suggested change

	/// Delete the saved configuration for the given wallet instead of listing.
	/// Delete the saved configuration for the given wallet.

[tvpeter]

Suggested change

	/// List all saved wallet configurations, or delete one with `--delete`.
	/// Saved wallet configuration operations.

[vadim-anfv]

cACK e987877

Thanks for picking this up. I left a couple of non-blocking comments inline, both of which fold naturally into the wallets list / wallets delete restructure @tvpeter asked for.

[yan-pi]

@tvpeter, that makes sense. Splitting the operations would also fix the return type issue.

Just to confirm, should this become wallets list and wallets delete <name>, as @vadim-anfv suggested?

[tvpeter]

@tvpeter, that makes sense. Splitting the operations would also fix the return type issue.

Just to confirm, should this become wallets list and wallets delete <name>, as @vadim-anfv suggested?

Yes. We both said the same thing.

[yan-pi]

Thanks for the first review!
I forgot to request another round of review. Now might be ready for it.

[vadim-anfv]

tACK f2a7e8f

Both of my earlier comments are addressed. I can't resolve the threads myself - could you close them?

Added two non-blocking notes inline.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.82%. Comparing base (b2b99b4) to head (5196d96).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers/config.rs	78.50%	23 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   59.06%   59.82%   +0.76%     
==========================================
  Files          22       22              
  Lines        3857     3968     +111     
==========================================
+ Hits         2278     2374      +96     
- Misses       1579     1594      +15     

Flag	Coverage Δ
rust	59.82% <80.00%> (+0.76%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tvpeter]

Thank you @yan-pi for working on this issue.

So having reconsidered the approach I suggested earlier, I think it may not be a good idea to delete saved configurations that have wallet data already persisted. So it will be better to handle it in such a way that, if the wallet data exists, return an error that wallet data exist for that configuration and do not delete the config. While it is possible to recreate the same config, the user might have lost the descriptors to do so. Also, if at all further down the line, we decide to consider deleting, it should also wipe the db file (for sqlite and much harder to even consider for redb) with a --force flag.

Aside that, kindly address the minor failing checks and review earlier by Vadim.

Thank you

[yan-pi]

The failing checks are unrelated to this PR.
The all-features build and Clippy stop during compilation on an unused import in src/handlers/payjoin/db.rs:537:

   use payjoin::persist::SessionPersister as _;

This file is not modified by this PR, and no Payjoin tests are executed. The default/no-default feature checks and formatting pass.

[yan-pi]

Rebased onto the latest upstream/master and addressed the requested changes. I believe it’s ready for another round of review.

[vadim-anfv]

Tested this locally and it looks good:

• an unused saved configuration is deleted, a wallet with persisted data is refused and keeps both its config entry and its data
• deleting one of two configurations leaves the other working
• deleting a name that is not in the config, and deleting with no config at all, both fail with a clear message
• redb: with two wallets in the shared store, the unused one is deleted while the store and the other wallet stay untouched
• builds clean with default features, --no-default-features and --all-features

Added one small suggestion inline, on the message shown when the shared redb store cannot be opened.

The branch conflicts with master in CHANGELOG.md though, could you rebase? I already ran all of the above against this head, so I'll tACK as soon as the conflict is gone.

[vadim-anfv]

The wallet being deleted may be sqlite-backed, so "failed to open Redb database" does not tell the user why the delete was refused.

Suggested change

	let database = bdk_redb::redb::Database::open(&db_path).map_err(|error| {
	Error::Generic(format!(
	"Failed to open Redb database at {db_path:?}: {error}"
	))
	})?;
	let database = bdk_redb::redb::Database::open(&db_path).map_err(|error| {
	Error::Generic(format!(
	"Cannot inspect the shared Redb store at {db_path:?} to tell whether '{wallet_name}' has data: {error}"
	))
	})?;