Feat/matching fee tracking #946

da-kami · 2023-07-13T11:21:10Z

Track matching fees in database and report on prometheus as counter.

holzeis

Nice 👍

coordinator/src/bin/coordinator.rs

holzeis · 2023-07-13T18:02:17Z

fixes: https://github.com/get10101/meta/issues/169

coordinator/migrations/2023-07-13-010300_trade_fees/up.sql

da-kami · 2023-07-13T23:57:16Z

I'm not merging this yet, I think it would be good if @bonomat and @luckysori can have a look :)

luckysori

I think this is well done!

I suggested a couple of things that are not so important and I left a few questions that other teammates might want to address.

coordinator/src/db/trades.rs

coordinator/src/node.rs

coordinator/src/trade/models.rs

coordinator/migrations/2023-07-13-010300_trade_fees/up.sql

luckysori · 2023-07-14T01:08:23Z

coordinator/src/metrics.rs

@@ -243,3 +257,48 @@ fn node_metrics(cx: &Context, inner_node: Arc<ln_dlc_node::node::Node<NodeStorag
        }
    }
 }
+
+pub fn collect_metrics_based_on_node_events(


❓We had a conversation about this yesterday, but I would still like to hear from others on why we think collecting metrics like this is necessary if they are already stored in the database. Cc @bonomat.

I don't see much benefit and this code needs to be maintained.

Context: @luckysori and me discussed if we should actually report this to prometheus at all. If we plan to use it for reporting we will created a report based on the database - what's the motivation to report it to prometheus?

I see these two different types of reports:

business metrics to track KPIs: these are in the DB and are collected manually once every X (e.g. 1/week)

liveness metrics: these are reported via prometheus to grafana so that we get a glimpse of what is happening.

We can combine these: for that I would not send events around but rather have a recurring thread that polls from the db and collects the data and prepares it for prometheus/grafana.
What do you think?

I completely agree with the DB based reports. That makes sense.

I am not totally convinced about reporting this particular metric to prometheus. Are we able to query the Postgres database from the Grafana board instead of reporting this via prometheus?

If we want to keep reporting this metric to prometheus I think it's better reporting it based on the event rather than a periodic task. If we have a periodic task that loads and reports fees it can happen that we miss reporting one (when we e.g. shut down the coordinator) unless we track what was already reported. This might create scenarios where we get confused because e.g. what is reported on prometheus does not match the report we got from the database. I am not convinced it's worth creating logic to track what we already reported; that's why I opted for reporting to prometheus based on the event.

As discussed offline just now: We won't report fees as metrics to Grafana (through Prometheus) because there is nothing to act on here. Reporting to Prometheus will be removed from this PR.

coordinator/src/db/trades.rs

luckysori · 2023-07-14T01:12:27Z

coordinator/src/bin/coordinator.rs

+                    // Upon collab closing an expired position we cannot charge a fee using an
+                    // invoice. This dummy hash exists in the database to
+                    // represent zero-amount invoices.


❓Is this what we want to do, @bonomat? I'm not totally sure after reading this comment.

I actually wanted to try charging the user in this scenario as well, but this would require a more elaborate process in terms of messages. The coordinator currently triggers the close on the DLC subchannel level. We could change this so the coordinator proposes the close by sending an event over to the app. The app then triggers the close automatically - that way we would tap into the existing process and would charge the fee. I don't see this as part of this PR though.

My understanding is, that eventually we want to calculate the fee into the payout (and not charge it with a separate invoice); if we do this at some point this problem will go away.

Yea, both ways are better then what we have at the moment 😅
It would be nicer if the app would try to collab close an expired position when it detects it cc @holzeis

luckysori · 2023-07-14T01:20:08Z

coordinator/migrations/2023-07-13-010300_trade_fees/up.sql

+-- Your SQL goes here
+-- insert a payment with amount zero that is a dummy default payment that corresponds to this actual zero-amount invoice:
+-- lnbcrt0m1pj26wczdq50fjhymeqd9h8vmmfvdjsnp4qtwk40kf07d8fzlhdt2s9vqyeczarvk37safua4a0kz7wellkq3vjpp5d7dce9wzhfa3s4a3n7t4xu3ss9slah6sl6mc5ffzqqf45sv82ggqsp54cmyvxklz48ap60guqsln4wuh2ranlap3grg2djt5ykz7005fc6s9qyysgqcqpcxq8z7ps9nqepcz2l2s4qrp6qeks68qry55ylcz542c8a6m6fffmhlmhtn9nnf458ngv83pk0473xfecdk7m7maumqk4jvaymdg7fpgn9tujcehxpqqte97xd
+-- We insert this dummy payment so we are able to sum up fees on already existing trades and make the fee payment hash a mandatory field.


🙃 It's also for closing expired positions, at least at the moment.

Yeah, that was not planned initially - I can adapt the comment or even opt for having another dummy value in the db for that.
I thought about changing the model make the fee optional (removing the NOT NULL) constraint instead, but I feel eventually there should be a fee for every trade. Or not...? CC @bonomat :)

I'm not sure I fully understand the question: are you asking if there should be a fee for every trade? Imho the answer to this question is yes, if it is a trade, i.e. it remove liquidity from the orderbook, then there should be a fee involved.

luckysori · 2023-07-14T01:22:00Z

coordinator/src/bin/coordinator.rs

+                    // represent zero-amount invoices.
+                    let zero_amount_payment_hash_dummy = PaymentHash(
+                        <[u8; 32]>::from_hex(
+                            "6f9b8c95c2ba7b1857b19f975372308161fedf50feb78a252200135a41875210",


🔧 I think it would help if this is private to the db module. We could have a close_expired_position method without the fee_payment_hash. And maybe a NewTrade::close_expired encapsulating this stuff.

If we agree that the trade fee should always be set (but it is not for this particular case at the moment because it's not implemented) then I would keep this in the business logic. The database expects the payment hash to be set and should not "default" it (initial migration excepted, but that's a different case).

- Add different structs for insert / query - Don't expose the db model to the business logic - Make `position_id` mandatory field in the db (we cannot have a trade without a position)

da-kami · 2023-07-17T07:59:30Z

bors r+

bors · 2023-07-17T08:10:56Z

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.

949: Save routing fees in database r=da-kami a=da-kami fixes https://github.com/get10101/meta/issues/177 Notes: - I have not added the prometheus counter yet because I am waiting for the result of discussion here: #946 (comment) - Opted for a separate table in the databse instead of adding a table that abstracts over multiple "fees". I think it's better to created views / reports on top Co-authored-by: Daniel Karzel <[email protected]>

da-kami requested review from bonomat, holzeis and luckysori July 13, 2023 11:21

Base automatically changed from feat/coordinator-storage-payments to main July 13, 2023 11:33

holzeis approved these changes Jul 13, 2023

View reviewed changes

coordinator/src/bin/coordinator.rs Outdated Show resolved Hide resolved

da-kami force-pushed the feat/matching-fee-tracking branch from ebb2c68 to ea61df8 Compare July 13, 2023 23:53

da-kami commented Jul 13, 2023

View reviewed changes

coordinator/migrations/2023-07-13-010300_trade_fees/up.sql Show resolved Hide resolved

luckysori approved these changes Jul 14, 2023

View reviewed changes

da-kami mentioned this pull request Jul 14, 2023

Save routing fees in database #949

Merged

Align Trade model with Position model

3a1a93a

- Add different structs for insert / query - Don't expose the db model to the business logic - Make `position_id` mandatory field in the db (we cannot have a trade without a position)

da-kami force-pushed the feat/matching-fee-tracking branch 2 times, most recently from c70f478 to 404dfa1 Compare July 17, 2023 07:33

Save payment fee in database

044e726

da-kami force-pushed the feat/matching-fee-tracking branch from 404dfa1 to 044e726 Compare July 17, 2023 07:41

bors bot merged commit 117b145 into main Jul 17, 2023
7 checks passed

bors bot deleted the feat/matching-fee-tracking branch July 17, 2023 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/matching fee tracking #946

Feat/matching fee tracking #946

da-kami commented Jul 13, 2023

holzeis left a comment

holzeis commented Jul 13, 2023

da-kami commented Jul 13, 2023

luckysori left a comment

luckysori Jul 14, 2023

da-kami Jul 14, 2023

bonomat Jul 16, 2023

da-kami Jul 17, 2023 •

edited

Loading

da-kami Jul 17, 2023 •

edited

Loading

luckysori Jul 14, 2023

da-kami Jul 14, 2023

bonomat Jul 14, 2023

luckysori Jul 14, 2023

da-kami Jul 14, 2023

bonomat Jul 14, 2023

luckysori Jul 14, 2023

da-kami Jul 17, 2023

da-kami commented Jul 17, 2023

bors bot commented Jul 17, 2023

Feat/matching fee tracking #946

Feat/matching fee tracking #946

Conversation

da-kami commented Jul 13, 2023

holzeis left a comment

Choose a reason for hiding this comment

holzeis commented Jul 13, 2023

da-kami commented Jul 13, 2023

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

da-kami Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

da-kami commented Jul 17, 2023

bors bot commented Jul 17, 2023

da-kami Jul 17, 2023 •

edited

Loading

da-kami Jul 17, 2023 •

edited

Loading