chore(backend): remove payment balance accounts #197

wilsonianb · 2021-12-03T03:47:23Z

Changes proposed in this pull request

~~Send outgoing payments from and receive invoice payments to settlement accounts.~~
Outgoing payments and invoices have accounts for tracking the amounts
sent and received, but they do not represent (withdraw-able) balances.
Remove outgoing payments and invoices accounts used for tracking the amounts
sent and received.
Add ~~sendOutgoingPayment~~ fundOutgoingPayment mutation resolver. Wallet operators will be
notified when quote is complete (TODO) and call fundOutgoingPayment
after payment is approved (if necessary) and funds are reserved in
the sender's wallet account.
Make incoming and outgoing accounts distinct types
~~Rename Funding payment state as Ready~~
Replace web monetization service with Open Payments account spsp fallback tigerbeetle account. (All invoices are now Open Payments invoices.)
Do not update account balances when sending unfulfillable rate probe packets.

Context

Checklist

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

Outgoing payments and invoices have accounts for tracking the amounts sent and received, but they do not represent (withdraw-able) balances. Add sendOutgoingPayment mutation resolver. Wallet operators will be notified when quote is complete (TODO) and call sendOutgoingPayment after payment is approved (if necessary) and funds are reserved in the sender's wallet account.

Rename transferFunds to sendAndReceive

sentientwaffle · 2021-12-06T19:26:20Z

packages/backend/src/connector/core/middleware/balance.ts

-            accounts.outgoing.id
+          assert.ok(accounts.outgoing.receivedAccountId)
+          const receiveLimit = await services.accounts.getBalance(
+            accounts.outgoing.receivedAccountId


The naming of receivedAccountId confuses me. It isn't the amount that has been received, it is the maximum amount that can be received, right?

It's confusing because it depends:
https://github.com/interledger/rafiki/pull/197/files#diff-e5a9751319994ef97d1413f0c3bc6587f756163958ad21f877a5fe7d84a08661R81-R89

I considered having separate receivedAccountId and receiveLimitAccountId, but ~~transferFunds~~sendAndReceive would treat them the same.

sentientwaffle · 2021-12-06T21:38:42Z

packages/backend/src/graphql/schema.graphql

@@ -302,6 +304,7 @@ type PaymentQuote {
  minExchangeRate: Float!
  lowExchangeRateEstimate: Float!
  highExchangeRateEstimate: Float!
+  amountSent: UInt64!


Why does this parameter need to be exposed on the API?

If it does need to be exposed, it should have a more descriptive name to ensure that the caller doesn't assume that it should be subtracted from (e.g.) the maxSourceAmount.

It doesn't need to be exposed since it's in the OutgoingPaymentOutcome

wilsonianb · 2021-12-08T17:39:05Z

@matdehaast Here's the possible webhooks for reference #199

matdehaast

This is good work @wilsonianb.

I do still worry that we are losing some accounting principals and accurate tracking of money movement by going in this direction. But I can see that it does simplify things greatly.

My assumption is that this is all to account for the need to enforce limits on certain payment accounts?

I wonder if there is a better abstraction where we can have another primitive within the account service that is called a limit account or something along those lines and we could bind that to an account. I'm trying to draw up some T accounts for this PR to see if we actually lose anything meaningful.

matdehaast · 2021-12-09T10:01:31Z

packages/backend/src/accounting/accounts.ts

-          : AccountFlags.debits_must_not_exceed_credits,
+          : account.type === AccountType.Credit
+          ? AccountFlags.debits_must_not_exceed_credits
+          : 0,


Does having a zero value here make sense? Should we not just define an account type enum for the case when its neither to remove ambiguity?

👍
A reason I didn't add and expose one is that currently the only non-Debit/Credit account is being created within the accounting service (in createAssetAccounts).

matdehaast · 2021-12-09T14:54:48Z

packages/backend/src/accounting/service.ts

+export type AccountOptions = (BasicAccountOptions | AssetAccountOptions) & {
+  receivedAccountId?: string
+  sentAccountId?: string
+}


Can an account have both a received and sent account id?

The reason I ask is, I'm wondering if its better to just have a singular account and then use a flag/enum/account type to determine if you are a received or sent account

It looks like this comment is on an outdated commit. Here's the final form:
https://github.com/interledger/rafiki/pull/197/files#diff-72a5530c2c89f105f75e071479a382e8f8408459d37968fb93b546cca4783b31R75-R91
There's currently not a situation in which an account would have both a received and sent account id.
With this previous iteration, it could have been consolidated to one account whose type could be checked, but I like how it fit into separating the connector's incoming and outgoing account types.
https://github.com/interledger/rafiki/pull/197/files#diff-08011ba8622ef9b1a38af778014f9471eb8ce942adc8609074f019765f7338f6R26-R43

matdehaast · 2021-12-09T14:58:21Z

packages/backend/src/open_payments/invoice/service.ts

+    // Invoice accounts are credited by the amounts received by the invoice.
+    //
+    // If amountToReceive is specified, the invoice account is initially
+    // debited by the amountToReceive, credits are restricted such that the
+    // invoice cannot receive more than that amount. The account balance
+    // represents the remaining receive limit.
+    //
+    // Otherwise, the invoice account balance represents the total amount
+    // received by the invoice.
    await deps.accountingService.createAccount({
      id: invoice.id,
      asset: invoice.account.asset,
-      type: AccountType.Credit,
-      receiveLimit: amountToReceive
+      type: amountToReceive ? AccountType.Debit : AccountType.Credit
    })


@wilsonianb are you worried this might be a footgun and catch someone else out?

Yes.
I'd like to separate web monetization from invoices and make the invoice model follow the open payments spec which would make amountToReceive a required field.

@wilsonianb ah I see that is a required field. I can't for the life of me remember why. I wonder how fixed send amounts are handled if amount is required...

Define how to do fixed source amount payments with invoices. open-payments#39

matdehaast · 2021-12-09T15:02:33Z

packages/backend/src/open_payments/invoice/routes.ts

+  const amountReceived = invoice.amountToReceive
+    ? invoice.amountToReceive + POSITIVE_SLIPPAGE - balance
+    : balance


I'm wondering if we should have a function in the invoice service rather to account for this, if somewhere else wants to quote the invoice balance, they will need to know this has to happen

I started to add a function yesterday and then I added getTotalReceived to the accountingService. 2ce7778
I'll put that in this pr.

Add SPSP fallback tigerbeetle account to Open Payments accounts.

wilsonianb · 2021-12-10T00:47:10Z

My assumption is that this is all to account for the need to enforce limits on certain payment accounts?

We were already effectively enforcing limits on payments.
This change simplifies limit enforcement (one tigerbeetle account per invoice and outgoing payment), while giving Rafiki fewer limits to enforce.

For outgoing payments, there's no longer a liquidity balance to enforce. The amount sent is restricted by @interledger/pay with its quote.

Invoices do still enforce the receive limit, but there's similarly no enforcement of an invoice's withdrawable liquidity. It has an amount received that can only go up, which the wallet can reflect in the receiver's wallet account however it wants.

wilsonianb · 2021-12-13T05:25:45Z

This won't work as is for an intra-rafiki same asset outgoing payment to invoice. It'll get TransferError.SameAccounts for the settlement account sending to itself.
I'm working on a change to fix.

Send and receive accounts have their own tigerbeetle account units, preventing transfers between them or with liquidity accounts (asset liquidity, settlement, and peer accounts). Fix same account error for same asset intra-rafiki transfers.

wilsonianb · 2021-12-15T02:47:46Z

☝️ that fixes the same account issue.
The pull request should be good to review.
@matdehaast @sentientwaffle

sentientwaffle · 2021-12-15T16:32:27Z

packages/backend/src/accounting/utils.ts


-const ASSET_ACCOUNTS_RESERVED = 32
+const ASSET_ACCOUNTS_RESERVED = 8


What is this constant for?

It's used in generating deterministic asset account ids:
https://github.com/interledger/rafiki/blob/main/packages/backend/src/accounting/utils.ts#L28-L30
We're now down to two types of asset accounts (liquidity & settlement).

I was actually just considering replacing it with:

const MAX_ASSETS = 2**16

based on Tigerbeetle's two byte unit field. And then getAssetAccountId could do:

BigInt(type * MAX_ASSETS + unit)

Couldn't miscellaneous account uuids get generated into that range?

Yeah, which is why I was still favoring ASSET_ACCOUNTS_RESERVED over MAX_ASSETS since the range is limited by the number of assets.

However, I think we would safely fail to create an asset if any of its tigerbeetle account ids are already used.
https://github.com/interledger/rafiki/blob/main/packages/backend/src/asset/service.ts#L57-L68
We'd end up with a unit sequence gap, and retrying would attempt to create the asset with the next unit value.

I also had the thought that validateId in accountingService.createAccount could disallow ids in the possible asset id range.

sentientwaffle · 2021-12-15T16:36:11Z

packages/backend/src/graphql/resolvers/outgoing_payment.ts

+  parent,
+  args,
+  ctx
+): ResolversTypes['OutgoingPaymentResponse'] => {


If the payment state is not Ready should that be a client (400) error?

Yeah, and maybe the same for requoteOutgoingPayment and cancelOutgoingPayment.

Should we define a WrongPaymentState error that the resolvers can catch and return 400?

Yeah. The alternative would be to check the state in the resolver itself, but I think that validation logic is better off in the service (where it already is).

Return 400 for unknown or wrong state payments.

wilsonianb · 2021-12-17T16:11:43Z

After talking with Matt, I'm in agreement that sending from and receiving to the settlement account prevents the wallet operator from getting an accurate snapshot of liabilities.
We also determined that we can still get by with a single tigerbeetle account for each payment:

Invoices can be deactivated when they've received their full amount (a la 9b68ea5). The limit doesn't need to be enforced by Tigerbeetle.
Outgoing payment total sent amount can be tracked with Tigerbeetle transfer codes. (since total debits won't work due to withdrawals)

I'll convert the payment accounts back to normal in this pr and do liquidity deposits/withdrawals and webhooks in separate pr(s).

Payments have a single liquidity account (no extra limit balances). Balances are no longer adjusted for unfulfillable rate probe packets.

Transition to Sending if funding provides adequate balance. Quoting transitions to Sending if balance is already sufficient. Remove sendOutgoingPayment. Rename Ready state to Funding.

sentientwaffle · 2021-12-20T18:48:42Z

packages/backend/src/open_payments/invoice/service.ts

+  const invoice = await Invoice.query(deps.knex).findById(invoiceId)
+  if (invoice.amount <= amountReceived) {
+    await invoice.$query(deps.knex).patch({
+      active: false
+    })


Would it cause problems if this query + patch are in separate transactions?

I don't think so since the amount currently isn't modified anywhere, but this could be put in a single

await Invoice.query(deps.knex).patch({ active: false }).where('id', invoiceId).andWhere('amount', '<=', amountReceived)

I ran into objection/knex type errors trying to compare with a bigint in andWhere so I've put the query and patch in a transaction 88ba6d1

Did you try passing the toString of the bigint?

Nope 🙃
6478831

sentientwaffle · 2021-12-20T18:53:51Z

packages/backend/src/outgoing_payment/service.ts

+    const payment = await OutgoingPayment.query(trx)
+      .findById(id)
+      .forUpdate()
+      .withGraphFetched('account.asset')


Can you double-check that the forUpdate doesn't also get carried over the the account.asset being fetched? (i.e. that the payment row is locked but not the account/asset). I know we do this elsewhere too but it didn't occur to me until now...

Vincit/objection.js#1608
I'm guessing it would for withGraphJoined

wilsonianb added 3 commits December 2, 2021 21:34

chore(backend): make incoming and outgoing accounts distinct types

7d65fa0

Rename transferFunds to sendAndReceive

chore(backend): rename Funding payment state Ready

9edf1bb

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related labels Dec 3, 2021

wilsonianb marked this pull request as ready for review December 3, 2021 03:53

wilsonianb requested review from sentientwaffle, sharafian, dclipp, matdehaast and sabineschaller December 3, 2021 03:54

sentientwaffle reviewed Dec 6, 2021

View reviewed changes

chore(backend): remove quote amountSent from query resolver

55da10b

sentientwaffle previously approved these changes Dec 7, 2021

View reviewed changes

matdehaast reviewed Dec 9, 2021

View reviewed changes

wilsonianb added 8 commits December 9, 2021 14:27

chore(backend): add getTotalSent/getTotalReceived

344014c

chore(backend): accounts -> accounting in connector

4fbc595

chore(backend): remove web monetization service

65a5b00

Add SPSP fallback tigerbeetle account to Open Payments accounts.

chore(backend): match Open Payments invoice spec

9144037

chore(backend): add AccountType.Unrestricted

0578c64

chore(backend): add invoice getReceiveLimit

cc0000f

chore(backend): handleFunding -> handleReady

51266ca

chore(docs): update transaction-api.md

34052dd

wilsonianb dismissed sentientwaffle’s stale review via 34052dd December 10, 2021 00:32

wilsonianb requested review from matdehaast and sentientwaffle December 10, 2021 00:47

This was referenced Dec 10, 2021

Collect outgoing payment fee #168

Closed

Add outgoing payment liquidity mutation resolvers #160

Closed

Add invoice liquidity withdrawal mutation resolvers #191

Closed

chore(backend): invoice expiresAt required

741924f

sentientwaffle reviewed Dec 15, 2021

View reviewed changes

chore(backend): add OutgoingPaymentError

6b20485

Return 400 for unknown or wrong state payments.

sentientwaffle previously approved these changes Dec 15, 2021

View reviewed changes

wilsonianb marked this pull request as draft December 17, 2021 16:11

wilsonianb added 3 commits December 17, 2021 20:36

Merge branch 'main' into bw-rm-payment-account

161dbcc

chore(backend): remove send and receive accounts

808f912

Payments have a single liquidity account (no extra limit balances). Balances are no longer adjusted for unfulfillable rate probe packets.

feat(backend): add fundOutgoingPayment

c9ae748

Transition to Sending if funding provides adequate balance. Quoting transitions to Sending if balance is already sufficient. Remove sendOutgoingPayment. Rename Ready state to Funding.

wilsonianb dismissed sentientwaffle’s stale review via c9ae748 December 18, 2021 02:45

wilsonianb changed the title ~~chore(backend): remove payment accounts~~ chore(backend): remove payment balance accounts Dec 18, 2021

wilsonianb marked this pull request as ready for review December 18, 2021 02:52

chore(backend): deactivate fully paid invoice

3be8230

wilsonianb requested a review from sentientwaffle December 20, 2021 15:42

sentientwaffle reviewed Dec 20, 2021

View reviewed changes

wilsonianb added 2 commits December 20, 2021 14:03

chore(backend): deactivate paid invoice in transaction

88ba6d1

chore(backend): deactivate fully paid invoice in single query

6478831

sentientwaffle approved these changes Dec 20, 2021

View reviewed changes

wilsonianb merged commit 3fa6da7 into main Dec 20, 2021

wilsonianb deleted the bw-rm-payment-account branch December 20, 2021 23:33

wilsonianb mentioned this pull request Dec 21, 2021

Identify unfulfillable rate probe packets #205

Closed


		const ASSET_ACCOUNTS_RESERVED = 32
		const ASSET_ACCOUNTS_RESERVED = 8

chore(backend): remove payment balance accounts #197

chore(backend): remove payment balance accounts #197

Conversation

wilsonianb commented Dec 3, 2021 • edited Loading

Changes proposed in this pull request

Context

Checklist

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

wilsonianb commented Dec 8, 2021

matdehaast 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

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

wilsonianb Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb commented Dec 10, 2021 • edited Loading

wilsonianb commented Dec 13, 2021

wilsonianb commented Dec 15, 2021

Choose a reason for hiding this comment

wilsonianb Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb Dec 15, 2021 • 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

wilsonianb commented Dec 17, 2021

Choose a reason for hiding this comment

wilsonianb Dec 20, 2021 • 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

wilsonianb commented Dec 3, 2021 •

edited

Loading

wilsonianb Dec 13, 2021 •

edited

Loading

wilsonianb commented Dec 10, 2021 •

edited

Loading

wilsonianb Dec 15, 2021 •

edited

Loading

wilsonianb Dec 15, 2021 •

edited

Loading

wilsonianb Dec 20, 2021 •

edited

Loading