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

feat(backend): improve PSQL balance calculation #2881

Merged
merged 4 commits into from
Aug 26, 2024
Merged

feat(backend): improve PSQL balance calculation #2881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36efdd4
chore(backend): add check constraint to ledger transfers
mkurapov Aug 20, 2024
c5eca52
Merge branch 'main' into mk/better-balances
mkurapov Aug 20, 2024
2a61215
feat(backend): update balance calculation to use psql query
mkurapov Aug 21, 2024
fe5c167
Merge branch 'main' into 2879/mk/updated-balances
mkurapov Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/backend/migrations/20240820161040_add_ledger_transfer_constraint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.up = function (knex) {
return knex.schema.table('ledgerTransfers', function (table) {
table.check(
`("state" != 'PENDING') OR ("expiresAt" IS NOT NULL)`,
Copy link
Contributor

@BlairCurrey BlairCurrey Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mkurapov mkurapov Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mkurapov mkurapov Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rafiki.money db:

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

null,
'check_pending_requires_expires_at'
)
})
}

/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.down = function (knex) {
return knex.schema.table('ledgerTransfers', function (table) {
table.dropChecks(['check_pending_requires_expires_at'])
})
}
21 changes: 21 additions & 0 deletions packages/backend/src/accounting/psql/balance.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,27 @@ describe('Balances', (): void => {
})
})

test('ignores expired pending transfers', async (): Promise<void> => {
await createLedgerTransfer(
{
ledger: account.ledger,
creditAccountId: account.id,
debitAccountId: peerAccount.id,
state: LedgerTransferState.PENDING,
expiresAt: new Date(Date.now() - 1),
amount: 10n
},
knex
)

await expect(getAccountBalances(serviceDeps, account)).resolves.toEqual({
creditsPosted: 0n,
creditsPending: 0n,
debitsPosted: 0n,
debitsPending: 0n
})
})

describe('calculates balances for single transfers', (): void => {
const amounts = {
credit: {
Expand Down
65 changes: 35 additions & 30 deletions packages/backend/src/accounting/psql/balance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { LedgerAccount } from './ledger-account/model'
import { LedgerTransferState } from '../service'
import { ServiceDependencies } from './service'
import { getAccountTransfers } from './ledger-transfer'
import { TransactionOrKnex } from 'objection'

export interface AccountBalance {
Expand All @@ -16,38 +14,45 @@ export async function getAccountBalances(
account: LedgerAccount,
trx?: TransactionOrKnex
): Promise<AccountBalance> {
const { credits, debits } = await getAccountTransfers(
Copy link
Contributor

@BlairCurrey BlairCurrey Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mkurapov mkurapov Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

deps,
account.id,
undefined, // No limit for balances
trx
)
try {
const queryResult = await (trx ?? deps.knex).raw(
`
SELECT
COALESCE(SUM("amount") FILTER(WHERE "creditAccountId" = :accountId AND "state" = 'POSTED'), 0) AS "creditsPosted",
COALESCE(SUM("amount") FILTER(WHERE "creditAccountId" = :accountId AND "state" = 'PENDING'), 0) AS "creditsPending",
COALESCE(SUM("amount") FILTER(WHERE "debitAccountId" = :accountId AND "state" = 'POSTED'), 0) AS "debitsPosted",
COALESCE(SUM("amount") FILTER(WHERE "debitAccountId" = :accountId AND "state" = 'PENDING'), 0) AS "debitsPending"
FROM "ledgerTransfers"
WHERE ("creditAccountId" = :accountId OR "debitAccountId" = :accountId)
AND ("state" = 'POSTED' OR ("state" = 'PENDING' AND "expiresAt" > NOW()));
`,
{ accountId: account.id }
)

let creditsPosted = 0n
let creditsPending = 0n
let debitsPosted = 0n
let debitsPending = 0n

for (const credit of credits) {
if (credit.state === LedgerTransferState.POSTED) {
creditsPosted += credit.amount
} else if (credit.state === LedgerTransferState.PENDING) {
creditsPending += credit.amount
if (queryResult?.rows < 1) {
throw new Error('No results when fetching balance for account')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@BlairCurrey BlairCurrey Aug 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mkurapov mkurapov Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

for (const debit of debits) {
if (debit.state === LedgerTransferState.POSTED) {
debitsPosted += debit.amount
} else if (debit.state === LedgerTransferState.PENDING) {
debitsPending += debit.amount
const creditsPosted = BigInt(queryResult.rows[0].creditsPosted)
const creditsPending = BigInt(queryResult.rows[0].creditsPending)
const debitsPosted = BigInt(queryResult.rows[0].debitsPosted)
const debitsPending = BigInt(queryResult.rows[0].debitsPending)

return {
creditsPosted,
creditsPending,
debitsPosted,
debitsPending
}
}
} catch (err) {
deps.logger.error(
{
err,
accountId: account.id
},
'Could not fetch balances for account'
)
Comment on lines +48 to +54
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return {
creditsPosted,
creditsPending,
debitsPosted,
debitsPending
throw err
}
}
6 changes: 5 additions & 1 deletion packages/backend/src/tests/ledgerTransfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,13 @@ export const createLedgerTransfer = async (
creditAccountId: creditAccountId,
debitAccountId: debitAccountId,
amount: amount ?? 10n,
expiresAt,
ledger,
state: state ?? LedgerTransferState.POSTED,
expiresAt:
expiresAt ??
(state === LedgerTransferState.PENDING
? new Date(Date.now() + 86_400_000)
: undefined),
Comment on lines +43 to +45
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type
})
}
Loading