Fix/add fee provision 2 #395

kiwiidb · 2023-07-04T08:58:03Z

This PR ensures that there is no way an account can go below 0.

A new entry_type field is added to the transaction entry model to indicate the different types: incoming, outgoing, outgoing_reversal, fee_reserve, fee_reserve_reversal, fee.
When making an non-internal transaction the fee reserve is debited from the current account.
When the payment has completed, the fee_reserve entry is always reversed if there is one. This is done regardless if lndhub has restarted in the meantime or not.
The database constraint on unique tx entry tuples is updated to include the new entry_type field (because we now have identical entries under the previous schema).
An integration test has been added that would previously make an account go below 0.
Existing integration tests also needed to be updated because they assumed a standard payment has 2 transaction entries associated with it (outgoing + fee or outgoing + reversal), whereas it now has 4 transaction entries associated with it ( fee_reserve + fee_reserve reversal are always there in addition to the ones mentioned above).

To do:

fix integration tests
add new integration test to ensure this works as intended.

codecov · 2023-07-05T13:37:48Z

Codecov Report

Merging #395 (5a264de) into main (c9b394f) will decrease coverage by 0.35%.
The diff coverage is 61.59%.

@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
- Coverage   60.07%   59.73%   -0.35%     
==========================================
  Files          51       51              
  Lines        3264     3365     +101     
==========================================
+ Hits         1961     2010      +49     
- Misses       1097     1138      +41     
- Partials      206      217      +11

Impacted Files	Coverage Δ
lib/service/checkpayments.go	`50.49% <39.28%> (-6.65%)`	⬇️
lib/service/invoices.go	`68.07% <66.66%> (-2.48%)`	⬇️
integration_tests/lnd_mock_hodl.go	`82.60% <100.00%> (ø)`
lib/service/invoicesubscription.go	`59.81% <100.00%> (+0.18%)`	⬆️

... and 1 file with indirect coverage changes

bumi · 2023-07-10T06:43:59Z

db/models/transactionentry.go

@@ -14,9 +23,11 @@ type TransactionEntry struct {
 	ParentID        int64             `bun:",nullzero"`
 	Parent          *TransactionEntry `bun:"rel:belongs-to"`
 	CreditAccountID int64             `bun:",notnull"`
+	FeeReserve      *TransactionEntry `bun:"rel:belongs-to"`


how does this reference work? where is the FeeReserveID saved?

It's added there in https://github.com/getAlby/lndhub.go/blob/main/lib/service/checkpayments.go#L68.
The FeeReserveID is not saved on the transaction entry, I could not figure out how to get the id of the inserted record in https://github.com/getAlby/lndhub.go/blob/main/lib/service/invoices.go#L329 , I think because we're using a db transaction there it's not possible.

We're looking it up with its invoice id and entry_type = fee_reserve , and then adding it to its parent.

so you say this feeReserveEntry here does not have an Id: https://github.com/getAlby/lndhub.go/blob/main/lib/service/invoices.go#L329C34-L329C49

which then can be used in here: https://github.com/getAlby/lndhub.go/blob/main/lib/service/invoices.go#L311-L316 (if moved down)

by SQL it should be possible
and for documentation and proper references I would find it nice to have it, then the other query is also easier because we simply get the association.

if bun does not do that automatically then I think we can use returning
https://bun.uptrace.dev/guide/query-insert.html#api

with that we at least should have the inserted ID in the result: https://pkg.go.dev/database/sql#Result
which we then can use to set the foreign key.

so we save the fee reserve, get the ID of that entry and save that in the tx and all this in a transaction.
PG can do that.

bumi · 2023-07-10T06:45:33Z

db/migrations/20230703120000_add_tx_entry_type.up.sql

@@ -0,0 +1,2 @@
+alter table transaction_entries 
+add column entry_type character varying;


what's with old payments? I guess we can update that entry for those?

I think they can just be kept as they are, it seems a lot of work and not very useful to update.

I think it's inconsitent data that also requires special handling.
currently we only have incoming outgoing and fee transactions so the migration should be possible, e.g by iterating through the entries and checking the accounts.
I'd prefer not having old/inconsistent data if possible

bumi · 2023-07-10T06:48:22Z

lib/service/checkpayments.go


-    err := svc.DB.NewSelect().Model(&entry).Where("invoice_id = ?", id).Limit(1).Scan(ctx)
-    return entry, err
+	err := svc.DB.NewSelect().Model(&entry).Where("invoice_id = ? and entry_type = ?", id, models.EntryTypeOutgoing).Limit(1).Scan(ctx)


was theGetTransactionEntryByInvoiceIdalways returning the outgoing tx entry?

bumi · 2023-07-10T06:48:27Z

lib/service/checkpayments.go

+	if err != nil {
+		//migration issue: pre-feereserve payment will cause a "no rows in result set" error.
+		//in this case, we also look for the entries without the outgoing check, and do not add the fee reserve
+		//we can remove this later when all relevant payments will have an entry_type and a fee_reserve tx


imo we should update the data.
otherwise we can never get rid of this code handling old data.

I think we can get rid of it after 2 weeks because we won't encounter any more pre-feereserve payments in this code block by then.

it's a generic GetTransactionEntryByInvoiceId method so it should be independent of where it is used.
if we don't migrate the data this function will be broken when used elsewhere.

bumi · 2023-07-10T06:49:54Z

lib/service/invoices.go

-		Amount:          invoice.Amount,
+	feeAccount, err := svc.AccountFor(ctx, common.AccountTypeFees, userId)
+	if err != nil {
+		svc.Logger.Errorf("Could not find outgoing account user_id:%v", userId)


that should be the:

svc.Logger.Errorf("Could not find fee account for user_id:%v", userId)

bumi · 2023-07-10T06:55:07Z

lib/service/checkpayments.go

+	if err != nil {
+		return entry, err
+	}
+	entry.FeeReserve = &feeReserveEntry


why do we need to manually load this?

See #395 (comment)

bumi · 2023-07-10T06:57:12Z

lib/service/invoices.go

@@ -287,49 +293,142 @@ func (svc *LndhubService) HandleFailedPayment(ctx context.Context, invoice *mode
 	return err
 }

+func (svc *LndhubService) InsertTransactionEntry(ctx context.Context, invoice *models.Invoice, creditAccount, debitAccount, feeAccount models.Account) (entry models.TransactionEntry, err error) {


should we call this explicitly InsertOutgoingTransactionEntry
is EntryType always outgoing?

It might be better called InsertOutgoingTransactionEntries, yes.

kiwiidb added 23 commits July 3, 2023 14:19

add db migration, add entry types

000177f

add db migration

1313e08

fee reserves as seperate tx entries

dc54877

commit fee reserve tx entry

3660db3

fix cp error

6b149af

update constraint

f0237fa

also get fee reserve tx by invoice id

1c841b4

fix bug

4bd2613

remove debug

1677a97

remove fee reserve id from tx entry

9ba016b

add trick to avoid looking up the fee account again

c20f02e

fix comment c/p

a22ab04

fix bugs

c3bd3bc

wtf who wrote this

da3912f

boy scouting

c0c2f3e

fix hodl invoice test

809a6f9

no fee entry anymore for internal payments

61132a7

fix internal payment test

ef2f267

fix async payment test

ec0493c

fix async payment test

59ee273

fix async payment test

c09554a

fix async payment test

3527f44

fix async payment test

d8a9201

kiwiidb added 4 commits July 5, 2023 17:01

add test to go below 0 with hodl invoices

9a6f9ab

add test to show negative balances are no longer possible

3497d53

fix comment

8831541

clean up tables after 2 tests

1f2ffb9

kiwiidb marked this pull request as ready for review July 6, 2023 14:52

add another user so they don't interfere

24428b3

kiwiidb requested review from bumi and LucasRouckhout July 6, 2023 15:14

fix migration issue

5a264de

kiwiidb merged commit c46480d into main Jul 7, 2023
4 of 5 checks passed

kiwiidb deleted the fix/add-fee-provision-2 branch July 7, 2023 09:02

kiwiidb mentioned this pull request Jul 8, 2023

reserve fee amount in database #391

Closed

2 tasks

bumi reviewed Jul 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/add fee provision 2 #395

Fix/add fee provision 2 #395

kiwiidb commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

bumi Jul 11, 2023

bumi Jul 11, 2023

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

bumi Jul 10, 2023

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

bumi Jul 10, 2023

bumi Jul 10, 2023

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

bumi Jul 10, 2023

kiwiidb Jul 10, 2023

		@@ -0,0 +1,2 @@
		alter table transaction_entries
		add column entry_type character varying;

Fix/add fee provision 2 #395

Fix/add fee provision 2 #395

Conversation

kiwiidb commented Jul 4, 2023 • edited Loading

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

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

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

kiwiidb commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading