cashubtc/cdk
1
0
Fork 0
Code Issues 25 Pull requests 23 Projects Releases 43 Packages Wiki Activity Actions

Prepared Send #596

Merged
davidcaseria merged 63 commits from prepared-send into main 2025-03-20 11:44:44 +00:00
Conversation 31 Commits 63 Files changed 24 +1153 -434
davidcaseria commented 2025-02-11 01:45:30 +00:00 (Migrated from github.com)
Copy link

Description

Prepare sends by selecting proofs and returning a fee amount before finalizing a send into a token.

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

  • Unifies and optimizes the proof selection algorithm to use Wallet::select_proofs.
  • Wallet::send now requires a PreparedSend.
  • WalletDatabase proof state update functions have been consolidated into update_proofs_state.

ADDED

  • Sends should be initiated by calling Wallet::prepare_send.
  • A SendOptions struct controls optional functionality for sends.
  • Allow Amount splitting to target a fee rate amount.
  • Utility functions for Proofs.
  • Utility functions for SendKind.
  • Completed checked arithmetic operations for Amount (i.e., checked_mul and checked_div).

REMOVED

FIXED

Checklist

### Description <!-- Describe the purpose of this PR, what's being adding and/or fixed --> Prepare sends by selecting proofs and returning a fee amount before finalizing a send into a token. ----- ### Notes to the reviewers <!-- In this section you can include notes directed to the reviewers, like explaining why some parts of the PR were done in a specific way --> ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates <!-- Please do not edit the actual changelog but note what you changed here. --> #### CHANGED - Unifies and optimizes the proof selection algorithm to use `Wallet::select_proofs`. - `Wallet::send` now requires a `PreparedSend`. - `WalletDatabase` proof state update functions have been consolidated into `update_proofs_state`. #### ADDED - Sends should be initiated by calling `Wallet::prepare_send`. - A `SendOptions` struct controls optional functionality for sends. - Allow `Amount` splitting to target a fee rate amount. - Utility functions for `Proofs`. - Utility functions for `SendKind`. - Completed checked arithmetic operations for `Amount` (i.e., `checked_mul` and `checked_div`). #### REMOVED #### FIXED ---- ### Checklist * [ ] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [ ] I ran `just final-check` before committing
👍 1
ok300 (Migrated from github.com) reviewed 2025-02-11 01:45:30 +00:00
davidcaseria commented 2025-02-19 16:04:46 +00:00 (Migrated from github.com)
Copy link

@thesimplekid two areas I'm looking for feedback:

  1. How should we support CDK users who want to bring their own PreparedSend logic? I made PreparedSend a "private" structure so that users can't modify it, causing issues with our logic.
  2. How should we support memos? I have it in SendOptions but I think I should move it to be a parameter of the send function, possibly with a flag to include it in the Token or keep it internal with the eventual TransactionsDatabase.
@thesimplekid two areas I'm looking for feedback: 1. How should we support CDK users who want to bring their own `PreparedSend` logic? I made `PreparedSend` a "private" structure so that users can't modify it, causing issues with our logic. 2. How should we support memos? I have it in `SendOptions` but I think I should move it to be a parameter of the `send` function, possibly with a flag to include it in the `Token` or keep it internal with the eventual `TransactionsDatabase`.
thesimplekid (Migrated from github.com) reviewed 2025-02-21 16:36:17 +00:00
thesimplekid (Migrated from github.com) left a comment
Copy link

Great work on this PR.

How should we support CDK users who want to bring their own PreparedSend logic? I made PreparedSend a "private" structure so that users can't modify it, causing issues with our logic.

I think we can just added a PreparedSend::new that way they can at least create a PreparedSend to pass to send. But I think maybe better to not worry to much about this for now to keep it simpler and if that need arises we can revisit.

How should we support memos? I have it in SendOptions but I think I should move it to be a parameter of the send function, possibly with a flag to include it in the Token or keep it internal with the eventual TransactionsDatabase.
I agree I think we can move it to the send fn maybe we can have two and internal and an external (included in token) memo and when the former is not set use the external one.

Great work on this PR. > How should we support CDK users who want to bring their own PreparedSend logic? I made PreparedSend a "private" structure so that users can't modify it, causing issues with our logic. I think we can just added a `PreparedSend::new` that way they can at least create a `PreparedSend` to pass to `send`. But I think maybe better to not worry to much about this for now to keep it simpler and if that need arises we can revisit. > How should we support memos? I have it in SendOptions but I think I should move it to be a parameter of the send function, possibly with a flag to include it in the Token or keep it internal with the eventual TransactionsDatabase. I agree I think we can move it to the `send` fn maybe we can have two and internal and an external (included in token) memo and when the former is not set use the external one.
crates/cashu/src/nuts/nut07.rs
@ -33,3 +33,3 @@
Pending,
/// Proof is reserved
/// Reserved
///
thesimplekid (Migrated from github.com) commented 2025-02-21 13:31:26 +00:00
Copy link

This is not longer used when a proof is included in a token as that is PendingSpent

This is not longer used when a proof is included in a token as that is `PendingSpent`
crates/cashu/src/wallet.rs
@ -88,0 +105,4 @@
/// Check if send kind has tolerance
pub fn has_tolerance(&self) -> bool {
matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_))
}
thesimplekid (Migrated from github.com) commented 2025-02-21 13:32:42 +00:00
Copy link

Would it be useful is this returned the tolerance? Option<Amount> instead of a bool?

Would it be useful is this returned the tolerance? `Option<Amount>` instead of a bool?
crates/cdk/src/wallet/proofs.rs
thesimplekid (Migrated from github.com) commented 2025-02-21 15:11:27 +00:00
Copy link

Does this handle the edge case where the proof added to pay the fee increased the fee?

Does this handle the edge case where the proof added to pay the fee increased the fee?
thesimplekid (Migrated from github.com) commented 2025-02-21 15:46:18 +00:00
Copy link

I think this should be a Vec<Id> the nuts allow for mints to have multiple active keysets. Cdk mint limits this on the implementation level to only have one active keyset per unit, however on the wallet side I do not think we should assume this and should account for multiple as it is in spec.

I think this should be a `Vec<Id>` the nuts allow for mints to have multiple active keysets. Cdk mint limits this on the implementation level to only have one active keyset per unit, however on the wallet side I do not think we should assume this and should account for multiple as it is in spec.
crates/cdk/src/wallet/proofs.rs
@ -229,0 +351,4 @@
} else if right_amount >= amount {
return Self::select_least_amount_over(right, amount);
}
}
thesimplekid (Migrated from github.com) commented 2025-02-21 16:11:53 +00:00
Copy link

Does this logic only work for powers of two? This is more of a general question and not blocking as the currently cdk mostly assumes power of 2 keysets however this isn't actually required by the nuts so we should move away from it.

Does this logic only work for powers of two? This is more of a general question and not blocking as the currently cdk mostly assumes power of 2 keysets however this isn't actually required by the nuts so we should move away from it.
crates/cdk/src/wallet/send.rs
@ -35,0 +26,4 @@
opts: SendOptions,
) -> Result<PreparedSend, Error> {
tracing::info!("Preparing send");
thesimplekid (Migrated from github.com) commented 2025-02-21 15:43:09 +00:00
Copy link

This filter is required because of what you mentioned previously where the get_proofs will return proofs with conditions even if None is passed. I think we should update the get_proofs fn to handle this correctly to avoid the filter here.

I can do this as a small focused pr ahead of merging this. https://github.com/cashubtc/cdk/issues/611

This filter is required because of what you mentioned previously where the `get_proofs` will return proofs with conditions even if `None` is passed. I think we should update the `get_proofs` fn to handle this correctly to avoid the filter here. I can do this as a small focused pr ahead of merging this. https://github.com/cashubtc/cdk/issues/611
thesimplekid (Migrated from github.com) commented 2025-02-21 15:57:12 +00:00
Copy link
            if selected_total - amount > tolerance && opts.send_kind.is_offline() {
                return Err(Error::InsufficientFunds);
            }

We can just check the error case as if its okay we can allow it to continue to the internal_prepare_send below

```suggestion if selected_total - amount > tolerance && opts.send_kind.is_offline() { return Err(Error::InsufficientFunds); } ``` We can just check the error case as if its okay we can allow it to continue to the `internal_prepare_send` below
thesimplekid (Migrated from github.com) commented 2025-02-21 16:21:46 +00:00
Copy link

I may have missed it but was the proof set to Reserved in prepare_send. Am I correct that the expectation is the once a proof is selected as part of a prepared send it is reserved and cannot be used in other operations unless explicitly unreserved? This way prepare_send can be called multiple times without conflicting and we do not have to block other operations.

Can we add a doc comment to the send/prepare_send fn explaining this expectation.

I may have missed it but was the proof set to `Reserved` in `prepare_send`. Am I correct that the expectation is the once a proof is selected as part of a prepared send it is reserved and cannot be used in other operations unless explicitly unreserved? This way `prepare_send` can be called multiple times without conflicting and we do not have to block other operations. Can we add a doc comment to the send/prepare_send fn explaining this expectation.
crates/cdk/src/wallet/send.rs
@ -178,0 +212,4 @@
let keyset_fee_ppk = self.get_keyset_fees_by_id(active_keyset_id).await?;
tracing::debug!("Keyset fees: {:?}", keyset_fee_ppk);
// Calculate total send amount
thesimplekid (Migrated from github.com) commented 2025-02-21 16:02:27 +00:00
Copy link

What is the intention of force_swap is it just for the case where a wallet may want to rotate proofs for example from an inactive to active keyset?

What is the intention of `force_swap` is it just for the case where a wallet may want to rotate proofs for example from an inactive to active keyset?
crates/cdk/src/wallet/send.rs
thesimplekid (Migrated from github.com) commented 2025-02-21 16:23:32 +00:00
Copy link

Can we just add a few more lines explaining how the proofs_to_swap vs proofs_to_send should be uses and what they are?

Can we just add a few more lines explaining how the `proofs_to_swap` vs `proofs_to_send` should be uses and what they are?
crates/cdk/src/wallet/send.rs
thesimplekid (Migrated from github.com) commented 2025-02-21 16:25:09 +00:00
Copy link
    /// Include fee
    ///
    /// When this is true the token created will include the amount of fees needed to redeem the token (amount + fee_to_redeem)
```suggestion /// Include fee /// /// When this is true the token created will include the amount of fees needed to redeem the token (amount + fee_to_redeem) ```
crates/cdk/src/wallet/send.rs
thesimplekid (Migrated from github.com) commented 2025-02-21 16:26:27 +00:00
Copy link

I think the better ux is the default behavior is to include the fee always. Do we think that should be the default here in SendOptions or that should be done higher up the stack?

I think the better ux is the default behavior is to include the fee always. Do we think that should be the default here in `SendOptions` or that should be done higher up the stack?
crates/cdk/src/wallet/swap.rs
thesimplekid (Migrated from github.com) commented 2025-02-21 16:28:32 +00:00
Copy link

I think we should use Reserved here, and keep Pending as a state used by the mint ie in a melt operation where the ln payment is taking some time.

I think we should use `Reserved` here, and keep `Pending` as a state used by the mint ie in a melt operation where the ln payment is taking some time.
thesimplekid commented 2025-02-21 17:39:44 +00:00 (Migrated from github.com)
Copy link

Have you done any testing around does this change the proofs that would be selected from a set from previous cdk versions? Do you expect it to?

I would be interested to include some integrations tests on this so we don't change proofs selection accidentally.

Have you done any testing around does this change the proofs that would be selected from a set from previous cdk versions? Do you expect it to? I would be interested to include some integrations tests on this so we don't change proofs selection accidentally.
ok300 (Migrated from github.com) reviewed 2025-02-24 17:28:16 +00:00
crates/cashu/src/wallet.rs
@ -88,0 +105,4 @@
/// Check if send kind has tolerance
pub fn has_tolerance(&self) -> bool {
matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_))
}
ok300 (Migrated from github.com) commented 2025-02-24 16:47:30 +00:00
Copy link

Then has_tolerance could be merged with is_exact.

Then `has_tolerance` could be merged with `is_exact`.
crates/cdk-common/src/database/wallet.rs
@ -100,6 +92,8 @@ pub trait Database: Debug {
state: Option<Vec<State>>,
spending_conditions: Option<Vec<SpendingConditions>>,
) -> Result<Vec<ProofInfo>, Self::Err>;
ok300 (Migrated from github.com) commented 2025-02-24 17:06:11 +00:00
Copy link

Maybe instead of removing the 3 methods, we can keep a default implementation in the trait?

    /// Set proofs as pending in storage. Proofs are identified by their Y
    /// value.
    async fn set_pending_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> {
        self.update_proofs_state(ys, State::Pending).await
    }
    /// Reserve proofs in storage. Proofs are identified by their Y value.
    async fn reserve_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> {
        self.update_proofs_state(ys, State::Reserved).await
    }
    /// Set proofs as unspent in storage. Proofs are identified by their Y
    /// value.
    async fn set_unspent_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> {
        self.update_proofs_state(ys, State::Unspent).await
    }

This would avoid the need for wallet DBs to re-implement them (as it's done in cdk-rexie now) and generally preserve the simpler caller semantics from before.

Maybe instead of removing the 3 methods, we can keep a default implementation in the trait? ```rust /// Set proofs as pending in storage. Proofs are identified by their Y /// value. async fn set_pending_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> { self.update_proofs_state(ys, State::Pending).await } /// Reserve proofs in storage. Proofs are identified by their Y value. async fn reserve_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> { self.update_proofs_state(ys, State::Reserved).await } /// Set proofs as unspent in storage. Proofs are identified by their Y /// value. async fn set_unspent_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err> { self.update_proofs_state(ys, State::Unspent).await } ``` This would avoid the need for wallet DBs to re-implement them (as it's done in `cdk-rexie` now) and generally preserve the simpler caller semantics from before.
crates/cdk/src/wallet/keysets.rs
ok300 (Migrated from github.com) commented 2025-02-24 16:56:33 +00:00
Copy link
        self.get_keyset_fees()
            .await?
            .get(&keyset_id)
            .cloned()
            .ok_or(Error::UnknownKeySet)

This can re-use the get_keyset_fees method above.

```suggestion self.get_keyset_fees() .await? .get(&keyset_id) .cloned() .ok_or(Error::UnknownKeySet) ``` This can re-use the `get_keyset_fees` method above.
crates/cdk/src/wallet/send.rs
ok300 (Migrated from github.com) commented 2025-02-24 17:24:04 +00:00
Copy link

Shouldn't this check the options?

        match self.options.include_fee {
            true => self.swap_fee + self.send_fee,
            false => self.swap_fee,
        }
Shouldn't this check the options? ```suggestion match self.options.include_fee { true => self.swap_fee + self.send_fee, false => self.swap_fee, } ```
crates/cdk/src/wallet/send.rs
ok300 (Migrated from github.com) commented 2025-02-24 17:27:32 +00:00
Copy link

IMO it's better to set the SendOptions default here to true.

IMO it's better to set the `SendOptions` default here to `true`.
davidcaseria commented 2025-02-26 12:13:01 +00:00 (Migrated from github.com)
Copy link

Have you done any testing around does this change the proofs that would be selected from a set from previous cdk versions? Do you expect it to?

I would be interested to include some integrations tests on this so we don't change proofs selection accidentally.

Yes, it should change proof selections to be more efficient. I wrote some unit tests because I made the select_proofs function static.

> Have you done any testing around does this change the proofs that would be selected from a set from previous cdk versions? Do you expect it to? > > > > I would be interested to include some integrations tests on this so we don't change proofs selection accidentally. Yes, it should change proof selections to be more efficient. I wrote some unit tests because I made the `select_proofs` function static.
davidcaseria (Migrated from github.com) reviewed 2025-02-26 12:17:21 +00:00
crates/cashu/src/wallet.rs
@ -88,0 +105,4 @@
/// Check if send kind has tolerance
pub fn has_tolerance(&self) -> bool {
matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_))
}
davidcaseria (Migrated from github.com) commented 2025-02-26 12:17:21 +00:00
Copy link

I prefer to use bools instead of checking the state of Options when doing conditional logic, such as how these functions are used. I can add a tolerance function that returns an Option, but I don't see why we shouldn't provide all of these functions in the API.

I prefer to use bools instead of checking the state of Options when doing conditional logic, such as how these functions are used. I can add a tolerance function that returns an Option, but I don't see why we shouldn't provide all of these functions in the API.
davidcaseria (Migrated from github.com) reviewed 2025-02-26 12:18:24 +00:00
crates/cdk/src/wallet/proofs.rs
davidcaseria (Migrated from github.com) commented 2025-02-26 12:18:24 +00:00
Copy link

I believe so. Do you have a specific edge case in mind that I can include as a test?

I believe so. Do you have a specific edge case in mind that I can include as a test?
davidcaseria (Migrated from github.com) reviewed 2025-02-26 13:46:26 +00:00
crates/cdk/src/wallet/send.rs
@ -178,0 +212,4 @@
let keyset_fee_ppk = self.get_keyset_fees_by_id(active_keyset_id).await?;
tracing::debug!("Keyset fees: {:?}", keyset_fee_ppk);
// Calculate total send amount
davidcaseria (Migrated from github.com) commented 2025-02-26 13:46:26 +00:00
Copy link

This is for when we need to send proofs with conditions. Any selected proofs must be swapped.

This is for when we need to send proofs with conditions. Any selected proofs must be swapped.
davidcaseria (Migrated from github.com) reviewed 2025-02-26 13:48:02 +00:00
crates/cdk/src/wallet/proofs.rs
@ -229,0 +351,4 @@
} else if right_amount >= amount {
return Self::select_least_amount_over(right, amount);
}
}
davidcaseria (Migrated from github.com) commented 2025-02-26 13:48:02 +00:00
Copy link

The logic should work for any keyset because of the fallback in finding the smallest amount over the target amount. However, it’s optimized for powers of two.

The logic should work for any keyset because of the fallback in finding the smallest amount over the target amount. However, it’s optimized for powers of two.
davidcaseria (Migrated from github.com) reviewed 2025-02-26 13:49:55 +00:00
crates/cdk/src/wallet/send.rs
davidcaseria (Migrated from github.com) commented 2025-02-26 13:49:55 +00:00
Copy link

Why? Typically, receivers (merchants) pay for fees.

Why? Typically, receivers (merchants) pay for fees.
davidcaseria (Migrated from github.com) reviewed 2025-02-26 15:16:44 +00:00
crates/cdk/src/wallet/send.rs
davidcaseria (Migrated from github.com) commented 2025-02-26 15:16:44 +00:00
Copy link

The send_fee should always be zero if include_fee == false. I think it's safer to keep as is.

The send_fee should always be zero if include_fee == false. I think it's safer to keep as is.
thesimplekid (Migrated from github.com) reviewed 2025-03-12 20:58:12 +00:00
crates/cdk/src/wallet/send.rs
@ -35,0 +26,4 @@
opts: SendOptions,
) -> Result<PreparedSend, Error> {
tracing::info!("Preparing send");
thesimplekid (Migrated from github.com) commented 2025-03-12 20:58:11 +00:00
Copy link

Sorry forgot I said I would look at this and got lost in the review.

This pr has a fix for matching the conditions using the current api which admittedly as a bit confusing and I'm open to suggestions on. But passing in None means that we do not care about that condition and want all proofs no matter what that field. For example get_proofs(None, None, None, None) would return ALL proofs in the db. get_proofs(Some("exmaple.xyz"), None, None, None would return all proofs of that mint. So the way that spending conditions is is None would but proofs of any or no spending condition. So to get proofs that do not have a spending condition it would be Some(vec![]). Firstly, Does how I've explained how this currently works make sense? Secondly, does this api make any sense or do you have a suggestion to improve it?

https://github.com/cashubtc/cdk/pull/654

Sorry forgot I said I would look at this and got lost in the review. This pr has a fix for matching the conditions using the current api which admittedly as a bit confusing and I'm open to suggestions on. But passing in `None` means that we do not care about that condition and want all proofs no matter what that field. For example `get_proofs(None, None, None, None)` would return **ALL** proofs in the db. `get_proofs(Some("exmaple.xyz"), None, None, None` would return all proofs of that mint. So the way that spending conditions is is `None` would but proofs of any or no spending condition. So to get proofs that do not have a spending condition it would be `Some(vec![])`. Firstly, Does how I've explained how this currently works make sense? Secondly, does this api make any sense or do you have a suggestion to improve it? https://github.com/cashubtc/cdk/pull/654
thesimplekid (Migrated from github.com) reviewed 2025-03-12 21:29:58 +00:00
crates/cdk/src/wallet/send.rs
thesimplekid (Migrated from github.com) commented 2025-03-12 21:29:58 +00:00
Copy link

Why? Typically, receivers (merchants) pay for fees.

This is true but in ecash i think this is going to lead to bad ux where someone says send me 25 sats you create a token for 25 sats without including fees and then when they receive they say why did you only send me 23 sats.

But as i alluded to above maybe this shouldn't be pushed on the lib level and the default of false makes more sense and wallet devs can choose the ux they want.

> Why? Typically, receivers (merchants) pay for fees. This is true but in ecash i think this is going to lead to bad ux where someone says send me 25 sats you create a token for 25 sats without including fees and then when they receive they say why did you only send me 23 sats. But as i alluded to above maybe this shouldn't be pushed on the lib level and the default of false makes more sense and wallet devs can choose the ux they want.
thesimplekid (Migrated from github.com) reviewed 2025-03-12 21:34:11 +00:00
crates/cdk-sqlite/src/wallet/migrations/20240612132920_init.sql
thesimplekid (Migrated from github.com) commented 2025-03-12 21:34:11 +00:00
Copy link

We need to create a new migration file. We can't just modify the init as anyone with an existing wallet will get errors.

I'm happy to push a commit fixing this if thats easier.

We need to create a new migration file. We can't just modify the init as anyone with an existing wallet will get errors. I'm happy to push a commit fixing this if thats easier.
thesimplekid (Migrated from github.com) reviewed 2025-03-12 21:37:44 +00:00
crates/cdk-sqlite/src/wallet/mod.rs
@ -734,6 +693,31 @@ FROM proof;
}
thesimplekid (Migrated from github.com) commented 2025-03-12 21:37:44 +00:00
Copy link

I know this is just following the bad example I set, but can we remove the for loop and do an in clause like Ive updated the mint to use.

github.com/cashubtc/cdk@72dff95322/crates/cdk-sqlite/src/mint/mod.rs (L1152-L1173)

I know this is just following the bad example I set, but can we remove the for loop and do an in clause like Ive updated the mint to use. https://github.com/cashubtc/cdk/blob/72dff953220768db129f5ce7d4c4fb337c6f838c/crates/cdk-sqlite/src/mint/mod.rs#L1152-L1173
thesimplekid commented 2025-03-12 21:45:55 +00:00 (Migrated from github.com)
Copy link

Not sure whats going on with the CI here it does not seem related to any of the code changes in this PR

Not sure whats going on with the CI here it does not seem related to any of the code changes in this PR
davidcaseria (Migrated from github.com) reviewed 2025-03-13 00:36:46 +00:00
crates/cdk-sqlite/src/wallet/migrations/20240612132920_init.sql
davidcaseria (Migrated from github.com) commented 2025-03-13 00:36:46 +00:00
Copy link

I forgot I did this, and I couldn't figure out how to update this check in a new migration. ChatGPT had me create a new table, transfer the data, delete the old table, and rename the new table. That didn't seem right so I just did this to get it to work. Do you know how to change the check?

I forgot I did this, and I couldn't figure out how to update this check in a new migration. ChatGPT had me create a new table, transfer the data, delete the old table, and rename the new table. That didn't seem right so I just did this to get it to work. Do you know how to change the check?
thesimplekid (Migrated from github.com) reviewed 2025-03-13 09:10:38 +00:00
crates/cdk-sqlite/src/wallet/migrations/20240612132920_init.sql
thesimplekid (Migrated from github.com) commented 2025-03-13 09:10:38 +00:00
Copy link

Yeah for sqlite that is what you have to do. The ALTER TABLE in sqlite is pretty limited if it was psotgress then we could just modify the existing table.

https://github.com/cashubtc/cdk/blob/main/crates/cdk-sqlite/src/mint/migrations/20240718203721_allow_unspent.sql

Yeah for sqlite that is what you have to do. The ALTER TABLE in sqlite is pretty limited if it was psotgress then we could just modify the existing table. https://github.com/cashubtc/cdk/blob/main/crates/cdk-sqlite/src/mint/migrations/20240718203721_allow_unspent.sql
davidcaseria (Migrated from github.com) reviewed 2025-03-14 13:20:40 +00:00
crates/cdk/src/wallet/send.rs
@ -35,0 +26,4 @@
opts: SendOptions,
) -> Result<PreparedSend, Error> {
tracing::info!("Preparing send");
davidcaseria (Migrated from github.com) commented 2025-03-14 13:20:40 +00:00
Copy link

My loosely held opinion is to keep the get_proofs API as is and add another function to return all proofs.

My loosely held opinion is to keep the `get_proofs` API as is and add another function to return all proofs.
thesimplekid (Migrated from github.com) reviewed 2025-03-14 14:13:59 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
thesimplekid (Migrated from github.com) commented 2025-03-14 14:13:47 +00:00
Copy link

Should we check the proof state here to make sure they are Reserved or Unspent and have not already been used in another tx

Should we check the proof state here to make sure they are Reserved or Unspent and have not already been used in another tx
crates/cdk/src/wallet/send.rs
@ -178,0 +183,4 @@
// Calculate swap fee
let swap_fee = self.get_proofs_fee(&proofs_to_swap).await?;
thesimplekid (Migrated from github.com) commented 2025-03-14 13:42:59 +00:00
Copy link

To get proofs with no conditions it should be.

 force_swap = true;
                available_proofs = self
                    .localstore
                    .get_proofs(
                        Some(self.mint_url.clone()),
                        Some(self.unit.clone()),
                        Some(vec![State::Unspent]),
                        Some(vec![]),
                    )
                    .await?;
To get proofs with no conditions it should be. ```rust force_swap = true; available_proofs = self .localstore .get_proofs( Some(self.mint_url.clone()), Some(self.unit.clone()), Some(vec![State::Unspent]), Some(vec![]), ) .await?; ```
crates/cdk/src/wallet/send.rs
@ -178,0 +215,4 @@
// Calculate total send amount
let total_send_amount = send.amount + send.send_fee;
tracing::debug!("Total send amount: {}", total_send_amount);
thesimplekid (Migrated from github.com) commented 2025-03-14 14:12:47 +00:00
Copy link

I see there is a cancel_send that sets the prepared proofs back to unspent but I do not see where the proofs are reserved, maybe I'm just missing it?

My understanding is that UNSPENT proofs should be selected and set to RESERVED in the prepare_send and then PreparedSend either needs to be used and proofs are set PENDINGSPENT or PreparedSend needs to be canceled and proofs are moved back to UNSPENT.

If we do not reserve the proofs in the prepare then if prepare is called again these proofs maybe selected again.

I see there is a `cancel_send` that sets the prepared proofs back to unspent but I do not see where the proofs are reserved, maybe I'm just missing it? My understanding is that `UNSPENT` proofs should be selected and set to `RESERVED` in the `prepare_send` and then `PreparedSend` either needs to be used and `proofs` are set `PENDINGSPENT` or `PreparedSend` needs to be canceled and proofs are moved back to `UNSPENT`. If we do not reserve the proofs in the prepare then if prepare is called again these proofs maybe selected again.
davidcaseria (Migrated from github.com) reviewed 2025-03-14 14:52:32 +00:00
crates/cdk/src/wallet/send.rs
@ -178,0 +215,4 @@
// Calculate total send amount
let total_send_amount = send.amount + send.send_fee;
tracing::debug!("Total send amount: {}", total_send_amount);
davidcaseria (Migrated from github.com) commented 2025-03-14 14:52:32 +00:00
Copy link

Somehow reserving the proofs got deleted. Adding a test now.

Somehow reserving the proofs got deleted. Adding a test now.
davidcaseria (Migrated from github.com) reviewed 2025-03-14 14:52:58 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
davidcaseria (Migrated from github.com) commented 2025-03-14 14:52:58 +00:00
Copy link

What error should be returned?

What error should be returned?
thesimplekid (Migrated from github.com) reviewed 2025-03-14 15:22:18 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
thesimplekid (Migrated from github.com) commented 2025-03-14 15:10:08 +00:00
Copy link

This will set all the proofs in the PreparedSend to Unspent. But we probably dont want to set proofs that are PendingSpent or Spent back. If the api is uses correctlly this wont be an issue but i think its best we check it.

This will set all the proofs in the PreparedSend to Unspent. But we probably dont want to set proofs that are PendingSpent or Spent back. If the api is uses correctlly this wont be an issue but i think its best we check it.
thesimplekid (Migrated from github.com) commented 2025-03-14 15:11:02 +00:00
Copy link

Maybe some new error ProofStateUnexpected and just add a log message.

Maybe some new error ProofStateUnexpected and just add a log message.
ok300 (Migrated from github.com) reviewed 2025-03-14 15:24:23 +00:00
crates/cdk/src/wallet/send.rs
@ -178,0 +215,4 @@
// Calculate total send amount
let total_send_amount = send.amount + send.send_fee;
tracing::debug!("Total send amount: {}", total_send_amount);
ok300 (Migrated from github.com) commented 2025-03-14 15:24:23 +00:00
Copy link

What happens if a caller forgets to call cancel_send? (or the app stops / crashes just before calling it)

Will those proofs remain RESERVED indefinitely?

What happens if a caller forgets to call `cancel_send`? (or the app stops / crashes just before calling it) Will those proofs remain `RESERVED` indefinitely?
davidcaseria (Migrated from github.com) reviewed 2025-03-14 15:27:21 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
davidcaseria (Migrated from github.com) commented 2025-03-14 15:27:21 +00:00
Copy link

Is this in case a bug is introduced? I can add the check for safety but I'm not sure how that would happen.

Is this in case a bug is introduced? I can add the check for safety but I'm not sure how that would happen.
davidcaseria (Migrated from github.com) reviewed 2025-03-14 16:07:24 +00:00
crates/cdk/src/wallet/send.rs
@ -178,0 +215,4 @@
// Calculate total send amount
let total_send_amount = send.amount + send.send_fee;
tracing::debug!("Total send amount: {}", total_send_amount);
davidcaseria (Migrated from github.com) commented 2025-03-14 16:07:24 +00:00
Copy link

Yes, until Wallet::unreserve_proofs is called.

Yes, until `Wallet::unreserve_proofs` is called.
thesimplekid (Migrated from github.com) reviewed 2025-03-15 10:02:28 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
thesimplekid (Migrated from github.com) commented 2025-03-15 10:02:28 +00:00
Copy link

Is this in case a bug is introduced? I can add the check for safety but I'm not sure how that would happen.

My thinking is to protect the user from the flow of prepared send -> send but then calling cancel on the prepared send. In this case where they've already created the token with send they should have to receive the token to reclaim it.

> Is this in case a bug is introduced? I can add the check for safety but I'm not sure how that would happen. My thinking is to protect the user from the flow of prepared send -> send but then calling cancel on the prepared send. In this case where they've already created the token with send they should have to receive the token to reclaim it.
davidcaseria (Migrated from github.com) reviewed 2025-03-17 12:51:53 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
davidcaseria (Migrated from github.com) commented 2025-03-17 12:51:53 +00:00
Copy link

This shouldn't be an issue because PreparedSend is not Clone, and both send and cancel_send move the struct, but I will add an additional check just in case.

This shouldn't be an issue because `PreparedSend` is not `Clone`, and both `send` and `cancel_send` move the struct, but I will add an additional check just in case.
thesimplekid (Migrated from github.com) reviewed 2025-03-17 16:23:33 +00:00
crates/cdk/src/wallet/send.rs
@ -45,135 +37,377 @@ impl Wallet {
}
thesimplekid (Migrated from github.com) commented 2025-03-17 16:23:33 +00:00
Copy link

Ah good point

Ah good point
thesimplekid (Migrated from github.com) approved these changes 2025-03-17 16:42:21 +00:00
thesimplekid (Migrated from github.com) left a comment
Copy link

ACK 8c7506f43c

ACK 8c7506f43c270576ea0adcef589c32c76222c87e
Sign in to join this conversation.
Reviewers
No reviewers
ok300
thesimplekid
Labels
Clear labels   
DB & Storage

   
Deployment

   
Error Handling & Logging

   
Maintenance

   
Payment Backend

   
backport

   
backport v0.13.x

   
backport v0.14.x

   
backport v0.15.x

   
bindings

   
blocked

   
bug

Something isn't working

  
cdk-sql

   
ci

   
cli

   
deps

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
keep-open

   
ldk-node-ui

   
migrations

   
mint

   
mutation-testing

   
needs rebase

   
needs review

   
new nut

This PR is held pending the merge of a new or updated NUT

  
nut change

   
question

Further information is requested

  
ready

   
rust-version

   
rustfmt

   
stacked hold

PR is based on top of another PR, so should be merged in sequence. See comments for details.

  
stale

   
testing

   
wallet

   
weekly-report

   
wontfix

This will not be worked on

No labels
DB & Storage
Deployment
Error Handling & Logging
Maintenance
Payment Backend
backport
backport v0.13.x
backport v0.14.x
backport v0.15.x
bindings
blocked
bug
cdk-sql
ci
cli
deps
documentation
duplicate
enhancement
good first issue
help wanted
invalid
keep-open
ldk-node-ui
migrations
mint
mutation-testing
needs rebase
needs review
new nut
nut change
question
ready
rust-version
rustfmt
stacked hold
stale
testing
wallet
weekly-report
wontfix
Milestone
Clear milestone
No items
No milestone
v0.8.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cashubtc/cdk!596
No description provided.