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.

Sign up for GitHub

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

Jump to bottom

Expose updated swap fees for user review #615

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Expose updated swap fees for user review #615

wants to merge 3 commits into from

Conversation

danielgranhao
Copy link
Contributor

@danielgranhao danielgranhao commented Dec 17, 2024 •
edited
Loading

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

roeierez
roeierez reviewed Dec 17, 2024
View reviewed changes
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
roeierez
roeierez reviewed Dec 18, 2024
View reviewed changes
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 •
edited
Loading

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kingonly kingonly kingonly left review comments

@roeierez roeierez roeierez left review comments

@dangeross dangeross dangeross left review comments

@hydra-yse hydra-yse Awaiting requested review from hydra-yse

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.

Zero-amount swap does not account for pair fee changes
4 participants
@danielgranhao @roeierez @dangeross @kingonly