Prepared Send #596
Prepared Send #596
Conversation
|
@thesimplekid two areas I'm looking for feedback:
|
There was a problem hiding this comment.
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 thesendfn 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.
| @@ -35,6 +35,8 @@ pub enum State { | |||
| /// | |||
| /// i.e. used to create a token | |||
There was a problem hiding this comment.
This is not longer used when a proof is included in a token as that is PendingSpent
| pub fn has_tolerance(&self) -> bool { | ||
| matches!(self, Self::OnlineTolerance(_) | Self::OfflineTolerance(_)) | ||
| } |
There was a problem hiding this comment.
Would it be useful is this returned the tolerance? Option<Amount> instead of a bool?
| selected_proofs.extend(Wallet::select_proofs( | ||
| remaining_amount, | ||
| remaining_proofs, | ||
| active_keyset_id, | ||
| &HashMap::new(), // Fees are already calculated | ||
| false, | ||
| )?); |
There was a problem hiding this comment.
Does this handle the edge case where the proof added to pay the fee increased the fee?
| .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(); |
There was a problem hiding this comment.
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
| amount: Amount, | ||
| proofs: Proofs, | ||
| active_keyset_id: Id, |
There was a problem hiding this comment.
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.
| // Update proofs state to pending spent | ||
| self.localstore | ||
| .update_proofs_state(proofs_to_send.ys()?, State::PendingSpent) | ||
| .await?; |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| /// Prepared send |
There was a problem hiding this comment.
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?
| pub amount_split_target: SplitTarget, | ||
| /// Send kind | ||
| pub send_kind: SendKind, | ||
| /// Include fee |
There was a problem hiding this comment.
| /// Include fee | |
| /// Include fee | |
| /// | |
| /// When this is true the token created will include the amount of fees needed to redeem the token (amount + fee_to_redeem) |
| /// Send kind | ||
| pub send_kind: SendKind, | ||
| /// Include fee | ||
| pub include_fee: bool, |
There was a problem hiding this comment.
I think the better ux is the default behavior is to include the fee always. Do we think that should be the default her in SendOptions or that should be done higher up the stack?
| self.localstore | ||
| .update_proofs_state(ys, State::Pending) | ||
| .await?; |
There was a problem hiding this comment.
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.
|
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. |
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
Wallet::select_proofs.Wallet::sendnow requires aPreparedSend.WalletDatabaseproof state update functions have been consolidated intoupdate_proofs_state.ADDED
Wallet::prepare_send.SendOptionsstruct controls optional functionality for sends.Amountsplitting to target a fee rate amount.Proofs.SendKind.Amount(i.e.,checked_mulandchecked_div).REMOVED
FIXED
Checklist
just final-checkbefore committing