Skip to content

chore: Removes clone from Wallet struct#1762

Open
TheMhv wants to merge 7 commits into
cashubtc:mainfrom
TheMhv:fix/remove-wallet-clone
Open

chore: Removes clone from Wallet struct#1762
TheMhv wants to merge 7 commits into
cashubtc:mainfrom
TheMhv:fix/remove-wallet-clone

Conversation

@TheMhv

@TheMhv TheMhv commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Description

It's a part from #1310

Removes clone from Wallet struct


Notes to the reviewers


Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED


Checklist

@github-project-automation github-project-automation Bot moved this to Backlog in CDK Mar 23, 2026
Comment thread crates/cdk/src/wallet/mod.rs Outdated
@github-project-automation github-project-automation Bot moved this from Backlog to In progress in CDK Mar 23, 2026
Comment thread crates/cdk-ffi/src/wallet.rs Outdated
Comment thread crates/cdk-integration-tests/src/init_pure_tests.rs
Comment thread crates/cdk-integration-tests/src/init_pure_tests.rs Outdated
Comment thread crates/cdk/examples/npubcash.rs Outdated
Comment thread crates/cdk-cli/src/sub_commands/update_mint_url.rs Outdated
@thesimplekid thesimplekid requested a review from crodas March 23, 2026 23:00
@thesimplekid

thesimplekid commented Mar 23, 2026

Copy link
Copy Markdown
Collaborator

- [x] We should also check with Zeus before merging this. They created their own binding for react native that may need clone.

Turns out I don't know how react native works and they somehow pull in the cdk bindings so as long as our bindings work its okay.

@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch 2 times, most recently from 5fa935d to e5d4993 Compare March 24, 2026 13:54
@TheMhv TheMhv marked this pull request as ready for review March 24, 2026 14:06
@TheMhv TheMhv requested a review from thesimplekid March 24, 2026 14:06
@TheMhv TheMhv changed the title Chore: Remove Wallet clone chore: Removes clone from Wallet struct Mar 24, 2026

@thesimplekid thesimplekid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks pretty good I think we just need to resolve #1762 (comment)

Comment thread crates/cdk-integration-tests/tests/wallet_saga.rs
@thesimplekid thesimplekid added this to the 0.17.0 milestone Mar 27, 2026
Comment thread crates/cdk/src/wallet/streams/npubcash.rs
@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch from e5d4993 to bf38f92 Compare March 29, 2026 15:18
@codecov

codecov Bot commented Mar 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 13.04348% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.46%. Comparing base (bfb5b8d) to head (2860afc).

Files with missing lines Patch % Lines
crates/cdk/src/wallet/wallet_repository.rs 18.75% 13 Missing ⚠️
crates/cdk-ffi/src/wallet_repository.rs 0.00% 2 Missing ⚠️
crates/cdk/src/wallet/npubcash.rs 0.00% 2 Missing ⚠️
crates/cdk/src/wallet/payment_request.rs 0.00% 2 Missing ⚠️
crates/cdk/src/wallet/streams/npubcash.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
- Coverage   71.48%   71.46%   -0.02%     
==========================================
  Files         356      356              
  Lines       73857    73862       +5     
==========================================
- Hits        52798    52787      -11     
- Misses      21059    21075      +16     

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

@ye0man ye0man moved this from In progress to In Review in CDK Mar 31, 2026
@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch from bf38f92 to 51d9ddb Compare April 2, 2026 19:11
.get_wallet(&sub_command_args.old_mint_url, unit)
.await?,
)
.expect("Unable to unwrap wallet"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This message is going to get bubbled to the user of the CLI. Probably something more descriptive would help in case of debugging in the future

@lescuer97

Copy link
Copy Markdown
Contributor

LGTM

@thesimplekid thesimplekid modified the milestones: 0.17.0, 0.18.0 Jun 1, 2026
@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch 2 times, most recently from 2e2642f to 41da877 Compare June 15, 2026 13:27

@cdk-bot cdk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified findings approved for disclosure:

  • update-mint-url panics when unwrapping repository Arc (medium) - The cdk-cli update-mint-url subcommand panics instead of updating the mint URL for repository-managed wallets, making the command unusable after this PR's Arc refactor.
    Unanchored locations included in summary:
    • crates/cdk/src/wallet/wallet_repository.rs:290

Comment thread crates/cdk-cli/src/sub_commands/update_mint_url.rs Outdated
@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch from 41da877 to 2860afc Compare June 16, 2026 13:55

@cdk-bot cdk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified findings approved for disclosure:

  • Repository mint URL update does not migrate persisted wallet data (high) - Updating a mint URL through the repository/CLI leaves the wallet database under the old URL, so the newly returned wallet can no longer see existing persisted balance/quotes for the mint.

Comment thread crates/cdk/src/wallet/wallet_repository.rs
@TheMhv TheMhv force-pushed the fix/remove-wallet-clone branch from 2860afc to 47eb3a1 Compare June 16, 2026 22:24

@cdk-bot cdk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified findings approved for disclosure:

  • update_mint_url drops the wallet if another Arc reference exists (medium) - If any consumer holds an Arc while updating a mint URL, the update fails and the repository drops the wallet from its in-memory map until it is recreated or the repository is reloaded.

let mut wallets = self.wallets.write().await;

let key = WalletKey::new(mint_url.clone(), unit.clone());
let mut wallet = Arc::try_unwrap(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

update_mint_url removes the wallet from the repository before attempting Arc::try_unwrap. Since the PR now exposes repository wallets as Arc<Wallet>, any normal caller that kept the value returned by create_wallet/get_wallet holds an additional strong reference. In that case try_unwrap fails, this method returns UnknownWallet, and the old entry has already been removed from self.wallets, so subsequent repository lookups lose the in-memory wallet.

Consider avoiding try_unwrap here, or at minimum restoring the removed Arc on the error path. A design with interior mutability / recreating and replacing the map entry without requiring unique Arc ownership would also avoid invalidating active wallet handles.

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

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

6 participants