Pay order-matching fee on the app #863

luckysori · 2023-06-29T10:13:36Z

Fixes #838¹.
Fixes https://github.com/get10101/meta/issues/170.
Fixes https://github.com/get10101/meta/issues/171.
Fixes #884.

We keep it simple for now: the coordinator returns an invoice and we trust the app to pay for it.

Invoice generation rationale

We choose to let the coordinator generate
an invoice rather than send an spontaneous payment from the app so that we can more easily update the coordinator fees without having to release a new version of the app.

Payment timing rationale

In patch 7c6a4bb the app was paying the invoice as soon as possible, but this had two downsides:

The app could end up paying for a trade that didn't come to fruition.
Updating the LN-DLC channel from the rust-dlc side and rust-lightning side simultaneously is not currently supported, so we could run into channel-breaking issues.

As such, we eventually opted for the simple 74a0acf. The solution is not perfect and it assumes that the app will not be rebooted until the payment is sent, but it should be good enough for now.

Other thoughts

Eventually, the order-matching fee could be included in the payout curve or, perhaps even better, as a separate static output on the buffer transaction (this is currently not supported).

These are some TODOs that could be tackled here or in follow-ups:

Verify that the app can actually pay for both executing the trade and the order-matching fee before executing the trade.
Only claim the order-matching fee once the trade has been executed. Only claim the order-matching fee once the trade has been executed #884
Tell the user about the estimated fee to be charged before they create the order. https://github.com/get10101/meta/issues/170
Show the user the charged fee on the success screen after executing the order. https://github.com/get10101/meta/issues/171

Given that we can only charge taker fees for now. We should create a separate ticket for the maker once it's possible to do so. ↩

luckysori · 2023-06-29T10:28:56Z

With 4a18708 it looks like this:

(Ignore the overflow warnings, those are already happening on native.)

coordinator/src/node/order_matching_fee.rs

da-kami · 2023-06-30T00:51:23Z

Verify that the app can actually pay for both executing the trade and the order-matching fee before executing the trade.

Not sure we have to - we can never really guarantee this completely because:

we have market orders and we cannot guarantee the execution price
The user might send funds out (we could block that, but that might also be hard.

I think the better solution will be:

The app (user) fails to pay the invoice for the order
The coordinator pushes a notification to the app, this will display a warning to the user that the invoice has to be paid otherwise consecutive orders will fail.
The coordinator has to keep record of the paymnets in association with the order. We could allow that the user pays the invoice from a different wallet to avoid being blacklisted (...)

da-kami · 2023-06-30T00:52:33Z

Tell the user about the estimated fee to be charged before they create the order.

In my opinion we should do this before we release this feature, otherwise it's not transparent enough.

luckysori · 2023-06-30T02:18:34Z

Verify that the app can actually pay for both executing the trade and the order-matching fee before executing the trade.

Not sure we have to - we can never really guarantee this completely because:

* we have market orders and we cannot guarantee the execution price

* The user might send funds out (we could block that, but that might also be hard.

I think the better solution will be:

1. The app (user) fails to pay the invoice for the order

2. The coordinator pushes a notification to the app, this will display a warning to the user that the invoice has to be paid otherwise consecutive orders will fail.

3. The coordinator has to keep record of the paymnets in association with the order. We could allow that the user pays the invoice from a different wallet to avoid being blacklisted (...)

Thanks for your thoughts on this.

I agree that we won't be able to fully guarantee that the taker (app) will be able to pay for both executing the trade and the fee invoice.

Since we want this to be atomic anyway and we know that we will eventually do so, I would advocate for keeping it as simple as possible for this non-atomic version. As such I would hesitate to add any complex logic around the current flow.

Given the current state of the project, I still advocate for a very crude check before letting the user place the order because:

We can take a reasonable estimate of the order-matching fee client-side, even if the price might fluctuate. We could be conservative and assume that the price might end up being 100 dollars worse, for example.
Fees should only be a small number of sats (given the trade sizes we are going for atm), so it's unlikely that we block a valid trade by mistake.

da-kami · 2023-06-30T03:07:23Z

[snip]

Given the current state of the project, I still advocate for a very crude check before letting the user place the order because:

1. We can take a reasonable estimate of the order-matching fee client-side, even if the price might fluctuate. We could be conservative and assume that the price might end up being 100 dollars worse, for example.

2. Fees should only be a small number of sats (given the trade sizes we are going for atm), so it's unlikely that we block a valid trade by mistake.

I'm happy with the crude check for now; given that we want to display the fee it should be part of the calculation anyway. (sorry, could have pointed that out)
As long as we are aware that we will need some tracking on the coordinator side at some point later, because we cannot guarantee the fee will get paid, that;s fine.

da-kami

Please see my comment, I am OK with merging this version, hence the approval, but I think changing it to pay after Finalized (upon updating the order to Filled) would be better.

We can also opt to experiment how often the user is charge without the execution succeeding, and if users actually complain.

mobile/native/src/ln_dlc/mod.rs

holzeis

Nice 👍

864: feat: charge fee when opening a jit channel r=holzeis a=holzeis fixes #577 - The payment of the fee is not yet depicted as a special payment. This will be covered in a follow up PR after aligning with #863 - The funding transaction is only cached in memory TODOs that will be covered in follow up tickets. - [ ] https://github.com/get10101/meta/issues/172 - [ ] #873 - [ ] #883 ![Simulator Screenshot - iPhone 14 Pro - 2023-06-30 at 11 34 28](https://github.com/get10101/10101/assets/382048/d22e680c-889a-4dd4-9e3f-c6d0d84a53ea) Co-authored-by: Richard Holzeis <[email protected]>

luckysori · 2023-07-05T13:07:36Z

I already had approvals before and now I've tackled the extra TODOs. Do take a look at the updated PR, @holzeis and @da-kami if you have time.

I'm merging for now. Any new feedback can be addressed in follow-ups.

bors r+

863: Pay order-matching fee on the app r=luckysori a=luckysori Fixes #838[^1]. Fixes https://github.com/get10101/meta/issues/170. Fixes https://github.com/get10101/meta/issues/171. Fixes #884. We keep it simple for now: the coordinator returns an invoice and we trust the app to pay for it. Invoice generation rationale ---------------------------- We choose to let the coordinator generate an invoice rather than send an spontaneous payment from the app so that we can more easily update the coordinator fees without having to release a new version of the app. Payment timing rationale ------------------------ In patch 7c6a4bb the app was paying the invoice as soon as possible, but this had two downsides: 1. The app could end up paying for a trade that didn't come to fruition. 2. Updating the LN-DLC channel from the `rust-dlc` side and `rust-lightning` side simultaneously is not currently supported, so we could run into channel-breaking issues. As such, we eventually opted for the simple 74a0acf. The solution is not perfect and it assumes that the app will not be rebooted until the payment is sent, but it should be good enough for now. Other thoughts -------------- Eventually, the order-matching fee could be included in the payout curve or, perhaps even better, as a separate static output on the buffer transaction (this is currently not supported). --- These are some TODOs that could be tackled here or in follow-ups: - [x] Verify that the app can actually pay for both executing the trade and the order-matching fee _before_ executing the trade. - [x] Only claim the order-matching fee once the trade has been executed. #884 - [x] Tell the user about the estimated fee to be charged before they create the order. https://github.com/get10101/meta/issues/170 - [x] Show the user the charged fee on the success screen after executing the order. https://github.com/get10101/meta/issues/171 [^1]: Given that we can only charge taker fees for now. We should create a separate ticket for the maker once it's possible to do so. Co-authored-by: Lucas Soriano del Pino <[email protected]>

We keep it simple for now: the coordinator returns an invoice and we trust the app to pay for it. Invoice generation rationale ---------------------------- We choose to let the coordinator generate an invoice rather than send an spontaneous payment from the app so that we can more easily update the coordinator fees without having to release a new version of the app. Payment timing rationale ------------------------ The app pays the invoice as soon as the coordinator responds to the trade request successfully. For now we let the coordinator claim the payment immediately, but we could wait until the trade has actually been executed to _claim_ the payment. Claiming the payment later should be simple enough: let the coordinator remember the `PaymentHash` of the order-matching fee invoice when generating it and do not claim it automatically when receiving the corresponding `PaymentClaimable` event (store the preimage instead). Once the trade has been executed, look for the invoice in storage and finally claim the funds. Other thoughts -------------- Eventually, the order-matching fee could be included in the payout curve or, perhaps even better, as a separate static output on the buffer transaction (this is currently not supported).

This patch includes an explicit sleep that should be removed once we understand why we can fail to emit the `PaymentClaimable` event in the coordinator and we implement a fix. Another caveat is that the order-matching fee invoice is stored in memory, so we could theoretically end up closing the app before the payment goes through.

bors · 2023-07-05T13:19:19Z

Canceled.

luckysori · 2023-07-05T13:21:19Z

bors r+

bors · 2023-07-05T13:36:53Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

luckysori requested review from bonomat, holzeis, da-kami and klochowicz June 29, 2023 10:13

luckysori self-assigned this Jun 29, 2023

luckysori force-pushed the feat/taker-fee branch from e0a05e2 to 528ad6c Compare June 29, 2023 10:22

luckysori commented Jun 29, 2023

View reviewed changes

coordinator/src/node/order_matching_fee.rs Outdated Show resolved Hide resolved

da-kami approved these changes Jun 30, 2023

View reviewed changes

mobile/native/src/ln_dlc/mod.rs Outdated Show resolved Hide resolved

holzeis approved these changes Jun 30, 2023

View reviewed changes

holzeis mentioned this pull request Jun 30, 2023

feat: charge fee when opening a jit channel #864

Merged

3 tasks

luckysori mentioned this pull request Jul 4, 2023

Only claim the order-matching fee once the trade has been executed #884

Closed

luckysori force-pushed the feat/taker-fee branch from 4a18708 to 23ca9e3 Compare July 5, 2023 12:58

luckysori added 7 commits July 5, 2023 23:19

feat(ui): Format trade-related wallet history entries

232cfbf

feat: Display order-matching fee estimate before trading

1eb9efb

fix(ui): Correct where the coins are locked up when trading

4143d4c

refactor(app): Use direction variable

f4007c7

feat(app): Consider fee when assessing usable balance

bb86063

luckysori force-pushed the feat/taker-fee branch from 23ca9e3 to bb86063 Compare July 5, 2023 13:19

bors bot merged commit dadbc4b into main Jul 5, 2023
7 checks passed

bors bot deleted the feat/taker-fee branch July 5, 2023 13:36

This was referenced Jul 5, 2023

How to charge closing fees without an order match #909

Closed

Charge order-matching fee for limit orders #910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pay order-matching fee on the app #863

Pay order-matching fee on the app #863

luckysori commented Jun 29, 2023 •

edited

Loading

luckysori commented Jun 29, 2023

da-kami commented Jun 30, 2023

da-kami commented Jun 30, 2023

luckysori commented Jun 30, 2023

da-kami commented Jun 30, 2023 •

edited

Loading

da-kami left a comment

holzeis left a comment

luckysori commented Jul 5, 2023

bors bot commented Jul 5, 2023

luckysori commented Jul 5, 2023

bors bot commented Jul 5, 2023

Pay order-matching fee on the app #863

Pay order-matching fee on the app #863

Conversation

luckysori commented Jun 29, 2023 • edited Loading

Invoice generation rationale

Payment timing rationale

Other thoughts

Footnotes

luckysori commented Jun 29, 2023

da-kami commented Jun 30, 2023

da-kami commented Jun 30, 2023

luckysori commented Jun 30, 2023

da-kami commented Jun 30, 2023 • edited Loading

da-kami left a comment

Choose a reason for hiding this comment

holzeis left a comment

Choose a reason for hiding this comment

luckysori commented Jul 5, 2023

bors bot commented Jul 5, 2023

luckysori commented Jul 5, 2023

bors bot commented Jul 5, 2023

luckysori commented Jun 29, 2023 •

edited

Loading

da-kami commented Jun 30, 2023 •

edited

Loading