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

Expose updated swap fees for user review #615

Merged
merged 20 commits into from
Jan 5, 2025

Conversation

danielgranhao
Copy link
Contributor

@danielgranhao danielgranhao commented Dec 17, 2024

Resolves #602

With this PR:

  • Users can provide an onchain fee rate leeway in sat/vbyte that they are fine with
  • When auto-accepting isn't possible, the payment gets into the new WaitingFeeAcceptance state. It will stay there until the user accepts the new fee, refunds the swap, or the swap expires
  • Listing payments waiting for review is done using the existing method list_payments
  • users can accept the new fees, proceeding with the payment. Otherwise, they can immediately refund the swap.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

I Very like the concept! Added some comments.

lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch from 5b9d84c to 97d6ca6 Compare December 18, 2024 00:35
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good to me, only brainstorming naming conventions.

lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
lib/bindings/src/breez_sdk_liquid.udl Outdated Show resolved Hide resolved
@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch 3 times, most recently from b71db0b to cb86f62 Compare December 20, 2024 13:36
@danielgranhao
Copy link
Contributor Author

danielgranhao commented Dec 20, 2024

This will need to be rebased on top of #620 in order for payments WaitingFeeAcceptance to be shown in the payments list. The core logic proposed here can already be reviewed though.

Also, I propose we update the notification plugins in a separate PR.

@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch from cb86f62 to ab9ee4b Compare December 20, 2024 15:44
@roeierez
Copy link
Member

@danielgranhao I did an initial pass and it looks solid to me, both the interface and the implementation. I still need to dig more into the code.
I think we need also to add a sync trigger when the payer_amount/receiver_amount changes now that we have the realtime sync in place.

@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch from ab9ee4b to 898629e Compare December 20, 2024 17:13
@danielgranhao
Copy link
Contributor Author

@roeierez Thanks! In the meantime, I've also identified some minor issues that need fixing. Once it's ready, I'll mark it.

@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch 2 times, most recently from 6643302 to 8a32e74 Compare December 21, 2024 01:46
@danielgranhao danielgranhao marked this pull request as ready for review December 21, 2024 01:54
@roeierez roeierez added this to the v0.6..0 milestone Dec 23, 2024
@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch 2 times, most recently from 5068779 to 401a025 Compare December 24, 2024 12:51
@danielgranhao danielgranhao marked this pull request as draft December 24, 2024 12:51
@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch from 401a025 to 0a2d9bb Compare December 24, 2024 16:36
@dangeross
Copy link
Collaborator

I think we should be able to recover the WaitingFeeAcceptance state, if there is only a user lockup tx and the swap payer/receive amount is 0

@danielgranhao
Copy link
Contributor Author

danielgranhao commented Dec 26, 2024

I think we should be able to recover the WaitingFeeAcceptance state, if there is only a user lockup tx and the swap payer/receive amount is 0

@dangeross Hm.. I don't think so. Those conditions would still apply even after the user accepts the fees, after which the state should be Pending. IIUC, the WaitingFeeAcceptance state requires info from the swapper service, and cannot be determined from onchain data and local state alone. But I could be missing something

@roeierez
Copy link
Member

I think we should be able to recover the WaitingFeeAcceptance state, if there is only a user lockup tx and the swap payer/receive amount is 0

@dangeross we had a discussion previously this week and decided to only transition to WaitingFeeAcceptance at the initiator end (local swap). This way the call to action will only be decided there and the rest of the synced devices will see the payment as pending. This is mainly for simplicity. Do you see any issue with that?

@danielgranhao danielgranhao marked this pull request as ready for review December 26, 2024 11:41
@dangeross
Copy link
Collaborator

Those conditions would still apply even after the user accepts the fees, after which the state should be Pending

If there is no server lockup tx, a user lockup tx and 0 amount it could be determined to be WaitingFeeAcceptance. Once accepted the payer/receiver amounts are updated and we should see a server lockup tx, then we could move on to Pending

@dangeross
Copy link
Collaborator

Do you see any issue with that?

I can't think of an issue

@danielgranhao
Copy link
Contributor Author

If there is no server lockup tx, a user lockup tx and 0 amount it could be determined to be WaitingFeeAcceptance

The issue is we want to update the amount before moving to WaitingFeeAcceptance so that we can show the user the value of the payment waiting for fee acceptance. We would have to persist some additional data, like the fact that fees have been accepted, in order to distinguish between both states. Maybe that's the way forward?

@danielgranhao danielgranhao force-pushed the updated-swap-fees-user-review branch from 6f9514c to a020ed5 Compare January 2, 2025 10:40
lib/core/src/sync/model/data.rs Show resolved Hide resolved
lib/core/src/model.rs Show resolved Hide resolved
lib/core/src/chain_swap.rs Show resolved Hide resolved
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.

LGTM

@danielgranhao danielgranhao requested a review from roeierez January 3, 2025 10:28
@danielgranhao danielgranhao merged commit 6d06580 into main Jan 5, 2025
9 checks passed
@danielgranhao danielgranhao deleted the updated-swap-fees-user-review branch January 5, 2025 22:18
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.

Zero-amount swap does not account for pair fee changes
5 participants