-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add zero-amount Receive Chain Swap #538
base: main
Are you sure you want to change the base?
Conversation
55937a5
to
9a82900
Compare
9f41dab
to
89e20d4
Compare
The SDK user has to provide a fee rate for these refunds, so they need to be manually handled |
# Conflicts: # lib/core/src/sdk.rs
It's not about refunds per se. The zero-amount swaps get the However it will not become an actual refundable when the user locks up funds, because when the zero-amount swap gets this event, we are getting and confirming the so-called quote from Boltz. By confirming it, we tell Boltz they should proceed with this swap (e.g. create the server lockup tx, etc). My question above was about this "getting and confirming" the quote. However we clarified in an earlier call that no user interaction should be needed to claim, so therefore we get and auto-confirm the quote. Hopefully this clarifies the confusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. some comments about the interface.
u64? payer_amount_sat; | ||
u64? zero_amount_min_payer_amount_sat; | ||
u64? zero_amount_max_payer_amount_sat; | ||
f64? zero_amount_service_feerate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix zero_amount is a bit confusing IMO.
Why do we need this prefix? Why not showing these values on all cases?
min/max and service fees seem reasonable to be shown also on cases where amount is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only weird case is when PrepareReceiveResponse.payment_method
is LiquidAddress
. Then min/max don't mean much.
Should I then set them to None
in that case?
} | ||
|
||
// We successfully accepted the quote, the swap should continue as normal | ||
return Ok(()); // Break from TxLockupFailed branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the error case arrives here too, we should allow it to be set to Refundable
let max = prepare_response.zero_amount_max_payer_amount_sat.unwrap(); | ||
let service_feerate = prepare_response.zero_amount_service_feerate.unwrap(); | ||
format!( | ||
"Fees: {fees} sat + {service_feerate}% of the sent amount. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the service feerate is going to be a 0.1
type value
@@ -1770,9 +1786,12 @@ impl LiquidSdk { | |||
} | |||
|
|||
/// Receive from a Bitcoin transaction via a chain swap. | |||
/// | |||
/// If no `user_lockup_amount_sat` is specified, this is an amountless swap and `fees_sat` exclude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If no `user_lockup_amount_sat` is specified, this is an amountless swap and `fees_sat` exclude | |
/// If no `payer_amount_sat ` is specified, this is an amountless swap and `fees_sat` exclude |
let swap = self | ||
.fetch_chain_swap_by_id(swap_id)? | ||
.ok_or_else(|| PaymentError::Generic { | ||
err: format!("Cannot update non-existent chain swap with ID: {swap_id}"), | ||
})?; | ||
ensure_sdk!( | ||
matches!(swap.direction, Direction::Incoming), | ||
PaymentError::Generic { | ||
err: format!( | ||
"Can only update payer_amount_sat for incoming chain swaps. Swap ID: {swap_id}" | ||
) | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a part of the SQL where clause below
.and_then(|quote| { | ||
info!("Got quote of {quote} sat for swap {}", &swap.id); | ||
|
||
self.persister.update_swap_payer_amount(&swap.id, quote)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the quote here is the server lockup amount. If so, I think there needs to be some recalculations done. The receiver amount (quote - claim fees), fees (server fees + boltz fees + claim fees) and payer amount (receiver amount + claim fees + server fees + boltz fees) need calculating.
Fixes #519
Open questions:
prepare_response
? How else to communicate them to caller, as they are now essential?lockupFailed
event)?payer_amount_sat
to 0 for zero-amount swaps? OrNULL
(DB migration)?