Prepared Send #596

davidcaseria · 2025-02-11T01:45:30Z

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

I followed the code style guidelines
I ran just final-check before committing

davidcaseria · 2025-02-19T16:04:46Z

@thesimplekid two areas I'm looking for feedback:

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

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

thesimplekid · 2025-02-21T13:32:42Z

crates/cashu/src/wallet.rs

+    pub fn has_tolerance(&self) -> bool {
+        matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_))
+    }


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

Then has_tolerance could be merged with is_exact.

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.

thesimplekid · 2025-02-21T15:11:27Z

crates/cdk/src/wallet/proofs.rs

+        selected_proofs.extend(Wallet::select_proofs(
+            remaining_amount,
+            remaining_proofs,
+            active_keyset_id,
+            &HashMap::new(), // Fees are already calculated
+            false,
+        )?);


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

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

thesimplekid · 2025-02-21T15:43:09Z

crates/cdk/src/wallet/send.rs

+                    .get_proofs(
+                        Some(self.mint_url.clone()),
+                        Some(self.unit.clone()),
+                        Some(vec![State::Unspent]),
+                        None,
+                    )
+                    .await?
+                    .into_iter()
+                    .filter_map(|p| {
+                        if p.spending_condition.is_none() {
+                            Some(p.proof)
+                        } else {
+                            None
+                        }
+                    })
+                    .collect();


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. #611

thesimplekid · 2025-02-21T15:46:18Z

crates/cdk/src/wallet/proofs.rs

        amount: Amount,
        proofs: Proofs,
+        active_keyset_id: Id,


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/send.rs

thesimplekid · 2025-02-21T16:26:27Z

crates/cdk/src/wallet/send.rs

+    /// Send kind
+    pub send_kind: SendKind,
+    /// Include fee
+    pub include_fee: bool,


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?

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

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

thesimplekid · 2025-02-21T16:28:32Z

crates/cdk/src/wallet/swap.rs

+        self.localstore
+            .update_proofs_state(ys, State::Pending)
+            .await?;


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 · 2025-02-21T17:39:44Z

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 · 2025-02-24T16:47:30Z

crates/cashu/src/wallet.rs

+    pub fn has_tolerance(&self) -> bool {
+        matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_))
+    }


Then has_tolerance could be merged with is_exact.

crates/cdk/src/wallet/keysets.rs

ok300 · 2025-02-24T17:06:11Z

crates/cdk-common/src/database/wallet.rs

-    async fn reserve_proofs(&self, ys: Vec<PublicKey>) -> Result<(), Self::Err>;
-    /// 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>;


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.

ok300 · 2025-02-24T17:24:04Z

crates/cdk/src/wallet/send.rs

+
+    /// Total fee
+    pub fn fee(&self) -> Amount {
+        self.swap_fee + self.send_fee


Shouldn't this check the options?

Suggested change

self.swap_fee + self.send_fee

match self.options.include_fee {

true => self.swap_fee + self.send_fee,

false => self.swap_fee,

}

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

ok300 · 2025-02-24T17:27:32Z

crates/cdk/src/wallet/send.rs

+    /// Send kind
+    pub send_kind: SendKind,
+    /// Include fee
+    pub include_fee: bool,


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

davidcaseria · 2025-02-26T12:13:01Z

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.

Co-authored-by: thesimplekid <[email protected]>

Co-authored-by: ok300 <[email protected]>

davidcaseria added 28 commits January 17, 2025 20:24

select_proofs_v2

39c40bd

select_proofs_v2 fix ups

7668b25

select_proofs_v2 edge case tests

d9365f3

select_proofs_v2 fix over test

10a150f

Merge remote-tracking branch 'origin/proof-select-v2' into prepared-send

1fa02aa

Prepared send

68caa7c

Fix swap amount

d26c7b6

WIP

e1f5cdb

Try fix

9b26914

Fix loop range modification bug

7c75483

Maybe this works

7a50e25

Fix split_with_fee

0d2925f

Refactor to pre-compute send_fee

e6820b8

Fix clippy errors

6d82e22

Add more comments

f9d18d1

Use new proof selection algorithm

878e9a9

Remove empty file

a999c5b

Export PreparedSend struct

d99e140

Include total fee in prepared send

ab29654

Fix swap amount

333741f

Fix swap amount

f51eabd

erge branch 'main' into prepared-send

6bcb512

Fix swap for including fees

fba73c3

Finally fix fees

a538e45

Fix returned swap amount

5dc9e31

Actually fix fees

781797e

Handle exact amounts

3fbc522

DRY ProofsMethods

7b2bdd0

davidcaseria marked this pull request as ready for review February 15, 2025 02:03

Ensure proofs are sorted before selecting least amount over

ff0778b

thesimplekid self-requested a review February 18, 2025 14:34

Allow memo to be set a time of send

09e0af0

davidcaseria added 3 commits February 19, 2025 16:01

Create SendMemo to use in SendOptions

92d3b58

Fix clippy

67e33e7

Fix fmt

0037705

thesimplekid reviewed Feb 21, 2025

View reviewed changes

ok300 reviewed Feb 24, 2025

View reviewed changes

davidcaseria and others added 8 commits February 26, 2025 10:05

Update crates/cdk/src/wallet/send.rs

b5514ea

Co-authored-by: thesimplekid <[email protected]>

Update crates/cdk/src/wallet/send.rs

2ef0155

Co-authored-by: thesimplekid <[email protected]>

Update crates/cdk/src/wallet/keysets.rs

5eb67a6

Co-authored-by: ok300 <[email protected]>

Address PR feedback

abdee5d

Fix clippy

a26ba5c

Merge remote-tracking branch 'upstream/main' into prepared-send

fc559a8

Fix doc to use no_compile

9cb7ac7

Merge remote-tracking branch 'upstream/main' into prepared-send

a19626c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepared Send #596

Prepared Send #596

davidcaseria commented Feb 11, 2025 •

edited

Loading

davidcaseria commented Feb 19, 2025

thesimplekid left a comment

thesimplekid Feb 21, 2025

ok300 Feb 24, 2025

davidcaseria Feb 26, 2025

thesimplekid Feb 21, 2025

davidcaseria Feb 26, 2025

thesimplekid Feb 21, 2025

thesimplekid Feb 21, 2025

thesimplekid Feb 21, 2025 •

edited

Loading

ok300 Feb 24, 2025

davidcaseria Feb 26, 2025

thesimplekid Feb 21, 2025

thesimplekid commented Feb 21, 2025

ok300 Feb 24, 2025

ok300 Feb 24, 2025

ok300 Feb 24, 2025

davidcaseria Feb 26, 2025

ok300 Feb 24, 2025

davidcaseria commented Feb 26, 2025

-        self.swap_fee + self.send_fee
+        match self.options.include_fee {
+            true => self.swap_fee + self.send_fee,
+            false => self.swap_fee,
+        }

Prepared Send #596

Are you sure you want to change the base?

Prepared Send #596

Conversation

davidcaseria commented Feb 11, 2025 • edited Loading

Description

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED

Checklist

davidcaseria commented Feb 19, 2025

thesimplekid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesimplekid Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesimplekid commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidcaseria commented Feb 26, 2025

davidcaseria commented Feb 11, 2025 •

edited

Loading

thesimplekid Feb 21, 2025 •

edited

Loading