[yanfsb]

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.

[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.

[yanfsb]

@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.

[yanfsb]

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 81.96721% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.74%. Comparing base (d34e6bf) to head (4445a26).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/persister.rs	75.00%	11 Missing ⚠️
src/config.rs	80.55%	7 Missing ⚠️
src/utils/common.rs	62.50%	3 Missing ⚠️
src/handlers/config.rs	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   57.97%   58.74%   +0.77%     
==========================================
  Files          25       25              
  Lines        4490     4608     +118     
==========================================
+ Hits         2603     2707     +104     
- Misses       1887     1901      +14     

Flag	Coverage Δ
rust	58.74% <81.96%> (+0.77%)	⬆️

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

[yanfsb]

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.

[yanfsb]

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.

[yanfsb]

Thanks!. Ive Rebased onto the latest master and:

• Added contextual errors when inspecting the shared Redb store, including the wallet name and database path.
• Added a regression test covering failures while inspecting the shared Redb store.
• And even evaluated extracting shared test helpers, but omitted it because the small line reduction caused unnecessary churn in already-reviewed tests.

[tvpeter]

Thank you @yan-pi for continuing to work on this.

Functionally, everything works as expected. The comments below focus on architectural separation: delegating persistence, config file edits, and error types to their designated modules. Once those are moved, you may find that some helper functions are redundant.

[yanfsb]

Thanks @tvpeter! Addressed these in c6cccd3 and rebased onto master.

[vadim-anfv]

tACK e6062c5

Re-ran the September scenarios on this head, all as expected:

• unused configuration deleted, config.toml removed with its last entry
• wallet with persisted data refused, entry and data kept
• deleting one of two leaves the other working, rewritten config.toml stays mode 600
• unknown name and missing config both fail clearly
• redb: unused wallet deleted, shared store and the other wallet intact; a sqlite wallet beside the store is unaffected; an unreadable store names the wallet, the path and the cause

Left one non-blocking comment inline, about a test that no CI job compiles.

[vadim-anfv]

ACK 4445a26

Only a rebase and the CI entry since my tACK on e6062c5, with git range-diff showing the original patches unchanged.

The new redb job now runs test_delete_rejects_foreign_sqlite_file_with_redb_marker, which nothing in the matrix compiled before.

[caarloshenriq]

tACK 4445a26

Saved three sqlite configs on regtest, deleted an unused one, got "not found" on repeat, and confirmed delete is refused after new_address persists data. Removing the last entry deletes config.toml. With redb, an unused wallet is deleted while another wallet's data lives in the shared wallet.redb, and the used one is refused

[tvpeter]

ACK 4445a26