Skip to content
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

Persist swapper fees for swaps #586

Merged
merged 8 commits into from
Dec 6, 2024
Merged

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Dec 1, 2024

This PR stores the swapper service fees for each swap type.

The service fee is reflected in the Payment in swapper_fees_sat. The Payment already contains the total fees in fees_sat, so from these two values a user can find the total swap network fees (total - service fees).

For chain swaps, this PR persists the service feerate instead of the service fee. In addition it also stores the server fees, which are a chain-swap-specific type of network fee (lockup tx fees paid by the server when locking up funds for the destination chain). These two new chain swap fields will later be used to validate the fees in Amountless Receive Chain Swaps.

@ok300 ok300 requested review from roeierez and dangeross December 1, 2024 17:38
@@ -631,6 +631,8 @@ pub(crate) struct ChainSwap {
pub(crate) payer_amount_sat: u64,
pub(crate) receiver_amount_sat: u64,
pub(crate) claim_fees_sat: u64,
pub(crate) swapper_service_feerate: f64,
pub(crate) swapper_server_fees_sat: u64,
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it makes sense to persist the pair as json in one field instead of breaking it (the same way we do with the create swap response)? It also contains other useful information that may help us validate things in the future such as limits and we can use the pair object functions later to validate fees the same way we do on creating the swap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ok300 ok300 requested a review from roeierez December 5, 2024 15:08
@ok300 ok300 merged commit 790dfa9 into main Dec 6, 2024
7 of 8 checks passed
Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

Sorry for the late review

Comment on lines +1275 to +1278
/// Service fees paid to the swapper service. This is only set for swaps (i.e. doesn't apply to
/// direct Liquid payments).
pub swapper_fees_sat: Option<u64>,

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if the swapper_fees_sat should go into the PaymentDetails, as they do only apply to Lightning and Bitcoin payments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Too late 😅, sorry my fault

Comment on lines +364 to +367
let swapper_fees_sat = maybe_chain_swap_pair_fees
.map(|pair| pair.fees.percentage)
.map(|fr| ((fr / 100.0) * payer_amount_sat as f64).ceil() as u64)
.unwrap_or(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wanted to question if the server fee for chain swaps should be considered as swapper fees, but I guess its part of the tx fee just like the claim fee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants