feat(backend): improve PSQL balance calculation

[mkurapov]

Changes proposed in this pull request

• For PSQL accounting, instead of calculating balances by fetching all transfers for an account and then doing the balance calculation in memory, do the calculation in PSQL
• Add constraint to ledgerAccounts table to prevent any PENDING transfers being created without an expiryDate (to avoid having "hanging" pending transfers that will never expire)

Running the perf test script with PSQL DB (graph showing transaction counts):

Before:

After:

Context

Fixes #2879

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Documentation added
• Make sure that all checks pass
• Bruno collection updated

[netlify]

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	fe5c167
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66c8b6c64360dd00088cc639

[mkurapov]

not doing much with the error here (not expecting it to happen), just logging the accountId we were trying to query

[mkurapov]

this is only used for tests, we just want to avoid breaking the newly added constraint above

[BlairCurrey]

We can tackle this in another issue but can we get rid of getAccountTransfers now? I think it's only exposed in a recently added gql resolver. That method still brings the records into code and iterate over all of them (so like half of the total problem you fixed here). So I would still expect performance issues as transfer count grows where its used (?).

IDK if we want to:

A) leave it as-is because we dont think the performance issue will matter much. It's possible. I dont think the use-case for the resolver is well defined. I guess its just so people can see each transfer from the admin ui (not currently implemented and not sure what they would do with that info).
B) Remove it
C) Also rework that query. Not exactly sure how much we would leverage this one since its aggregating it a bit differently.

I guess I would say B or C but im not sure its worth the trouble of refactoring if we dont have a clear use case for consuming it, so leaning B.

[mkurapov]

We can tackle this in another issue but can we get rid of getAccountTransfers now? I think it's only exposed in a recently added gql resolver

Yes, I think the gql is worth keeping and since its used more from dev/admin point of view and not something getting called every trx, its like you said, not a big deal, and we can leave as is for now.

Adding pagination (so we grab only a limited set of transactions) would be a good first step in improving it IMO (also just having a default low limit is not bad as well)

[BlairCurrey]

Does the new query error if some record doesnt satisfy the constraint (hence why you added it)? Just wondering if we can guaruntee all existing records will pass this constraint... migration will fail otherwise. Im also wondering if the error is intelligible if we try to add one that doesnt pass this constraint. Not really sure where that would happen or how it should surface...

[mkurapov]

Checking rafiki money would be a good test, but I think it's not actually something that should have ever happened, this is how we map all transfers before creating them:

rafiki/packages/backend/src/accounting/psql/ledger-transfer/index.ts

Lines 326 to 342 in 750f414

	function prepareTransfer(
	transfer: CreateLedgerTransferArgs
	): Partial<LedgerTransfer> {
	return {
	amount: transfer.amount,
	transferRef: transfer.transferRef,
	creditAccountId: transfer.creditAccount.id,
	debitAccountId: transfer.debitAccount.id,
	ledger: transfer.creditAccount.ledger,
	state: transfer.timeoutMs
	? LedgerTransferState.PENDING
	: LedgerTransferState.POSTED,
	expiresAt: transfer.timeoutMs
	? new Date(Date.now() + Number(transfer.timeoutMs))
	: undefined,
	type: transfer.type
	}

i.e. if the timeout is provided, we set PENDING with expiryAt always

In terms of error legibility, I think it should be properly serviced up when we create the transfers in the psql transaction.

I also think this is mostly something to protect against refactoring code - this would be more a dev error than anything

[mkurapov]

rafiki.money db:

rafiki_backend=# select count(*) from "ledgerTransfers" where state = 'PENDING' and "expiresAt" is null;
 count 
-------
     0
(1 row)

[golobitch]

I was always wondered how do we decide if we should throw error, or just return empty response (in case of a balance, 0)?

[BlairCurrey]

I think your question still stands but I played around with it because I wasnt sure how it would work and wanted to add some context. When there are no records with the associated id it still aggregates a result, its just 0 for all the balances. So I think like Max mentioned in his other comment, this could probably never happen. But yeah I think it still begs the question, do we need to error or can we just return 0 balances (logging for sure either way).

I guess since it's totally unexpected I'm not sure indicating the balances are 0 would be correct. Not entirely sure how this is consumed but it might send the wrong signal.

[mkurapov]

This check is if results of the query weren't able to be returned at all - so missing information unexpectedly, a result of a psql issue. So in that case we should throw and stop everything IMO