chore(backend): only call onCredit for destinationAccount

Remove onDebit. Remove TransferAccount. Update invoices index. Update transaction-api.md docs.
interledger · Feb 9, 2022 · 07661cf · 07661cf
1 parent e47695c
commit 07661cf
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 67 deletions.
diff --git a/docs/transaction-api.md b/docs/transaction-api.md
@@ -87,7 +87,7 @@ An expired invoice that has never received money is deleted.
 
 Rafiki sends webhook events to notify the wallet of payment lifecycle states that require liquidity to be added or removed.
 
-Webhook event handlers must be idempotent and return `200`.
+Webhook event handlers must be idempotent and return `200` on success. Rafiki will retry unsuccessful webhook requests for up to one day.
 
 ### `EventType`
 
@@ -185,13 +185,12 @@ The intent must include `invoiceUrl` xor (`paymentPointer` and `amountToSend`).
 
 ### `Invoice`
 
-| Name          | Optional | Type      | Description                                                                                                                    |
-| :------------ | :------- | :-------- | :----------------------------------------------------------------------------------------------------------------------------- |
-| `id`          | No       | `ID`      | Unique ID for this invoice, randomly generated by Rafiki.                                                                      |
-| `accountId`   | No       | `String`  | Id of the recipient's Open Payments account.                                                                                   |
-| `amount`      | No       | `UInt64`  | The amount that must be paid at the time the invoice is created, in base units of the account asset.                           |
-| `received`    | No       | `UInt64`  | The total amount received, in base units of the account asset.                                                                 |
-| `active`      | No       | `Boolean` | If `true`, the invoice may receive funds. If `false`, the invoice is either expired or has already received `amount` of funds. |
-| `description` | Yes      | `String`  | Human readable description of the invoice.                                                                                     |
-| `createdAt`   | No       | `String`  |                                                                                                                                |
-| `expiresAt`   | No       | `String`  |                                                                                                                                |
+| Name          | Optional | Type     | Description                                                                                          |
+| :------------ | :------- | :------- | :--------------------------------------------------------------------------------------------------- |
+| `id`          | No       | `ID`     | Unique ID for this invoice, randomly generated by Rafiki.                                            |
+| `accountId`   | No       | `String` | Id of the recipient's Open Payments account.                                                         |
+| `amount`      | No       | `UInt64` | The amount that must be paid at the time the invoice is created, in base units of the account asset. |
+| `received`    | No       | `UInt64` | The total amount received, in base units of the account asset.                                       |
+| `description` | Yes      | `String` | Human readable description of the invoice.                                                           |
+| `createdAt`   | No       | `String` |                                                                                                      |
+| `expiresAt`   | No       | `String` |                                                                                                      |
diff --git a/packages/backend/migrations/20220209103122_create_invoices_table.js b/packages/backend/migrations/20220209103122_create_invoices_table.js
@@ -17,7 +17,7 @@ exports.up = function (knex) {
 
     table.index(['accountId', 'createdAt', 'id'])
 
-    table.index('expiresAt')
+    table.index(['active', 'expiresAt'])
     /*
     TODO: The latest version of knex supports "partial indexes", which would be more efficient for the deactivateInvoice use case. Unfortunately, the only version of 'objection' that supports this version of knex is still in alpha.
     // This is a 'partial index' -- expiresAt is only indexed when active=true.

diff --git a/packages/backend/src/accounting/service.test.ts b/packages/backend/src/accounting/service.test.ts
@@ -8,7 +8,6 @@ import { v4 as uuid } from 'uuid'
 import {
   AccountingService,
   LiquidityAccount,
-  TransferAccount,
   Deposit,
   Withdrawal
 } from './service'
@@ -25,7 +24,7 @@ import {
   startTigerbeetleContainer,
   TIGERBEETLE_PORT
 } from '../tests/tigerbeetle'
-import { AccountFactory } from '../tests/accountFactory'
+import { AccountFactory, FactoryAccount } from '../tests/accountFactory'
 
 describe('Accounting Service', (): void => {
   let deps: IocContract<AppServices>
@@ -215,8 +214,8 @@ describe('Accounting Service', (): void => {
       ${true}   | ${'same asset'}
       ${false}  | ${'cross-currency'}
     `('$description', ({ sameAsset }): void => {
-      let sourceAccount: TransferAccount
-      let destinationAccount: TransferAccount
+      let sourceAccount: LiquidityAccount
+      let destinationAccount: FactoryAccount
       const startingSourceBalance = BigInt(10)
       const startingDestinationLiquidity = BigInt(100)
 

diff --git a/packages/backend/src/accounting/service.ts b/packages/backend/src/accounting/service.ts
@@ -45,7 +45,6 @@ export interface LiquidityAccount {
     unit: number
   }
   onCredit?: (balance: bigint) => Promise<LiquidityAccount>
-  onDebit?: (balance: bigint) => Promise<LiquidityAccount>
 }
 
 export interface Deposit {
@@ -58,15 +57,9 @@ export interface Withdrawal extends Deposit {
   timeout: bigint
 }
 
-export interface TransferAccount extends LiquidityAccount {
-  asset: LiquidityAccount['asset'] & {
-    asset: LiquidityAccount['asset']
-  }
-}
-
 export interface TransferOptions {
-  sourceAccount: TransferAccount
-  destinationAccount: TransferAccount
+  sourceAccount: LiquidityAccount
+  destinationAccount: LiquidityAccount
   sourceAmount: bigint
   destinationAmount?: bigint
   timeout: bigint // nano-seconds
@@ -227,34 +220,30 @@ export async function createTransfer(
     return TransferError.InvalidDestinationAmount
   }
   const transfers: Required<CreateTransferOptions>[] = []
-  const sourceAccounts: LiquidityAccount[] = []
-  const destinationAccounts: LiquidityAccount[] = []
 
   const addTransfer = ({
-    sourceAccount,
-    destinationAccount,
+    sourceAccountId,
+    destinationAccountId,
     amount
   }: {
-    sourceAccount: LiquidityAccount
-    destinationAccount: LiquidityAccount
+    sourceAccountId: string
+    destinationAccountId: string
     amount: bigint
   }) => {
     transfers.push({
       id: uuid(),
-      sourceAccountId: sourceAccount.id,
-      destinationAccountId: destinationAccount.id,
+      sourceAccountId,
+      destinationAccountId,
       amount,
       timeout
     })
-    sourceAccounts.push(sourceAccount)
-    destinationAccounts.push(destinationAccount)
   }
 
   // Same asset
   if (sourceAccount.asset.unit === destinationAccount.asset.unit) {
     addTransfer({
-      sourceAccount,
-      destinationAccount,
+      sourceAccountId: sourceAccount.id,
+      destinationAccountId: destinationAccount.id,
       amount:
         destinationAmount && destinationAmount < sourceAmount
           ? destinationAmount
@@ -265,15 +254,15 @@ export async function createTransfer(
       // Send excess source amount to liquidity account
       if (destinationAmount < sourceAmount) {
         addTransfer({
-          sourceAccount,
-          destinationAccount: sourceAccount.asset,
+          sourceAccountId: sourceAccount.id,
+          destinationAccountId: sourceAccount.asset.id,
           amount: sourceAmount - destinationAmount
         })
         // Deliver excess destination amount from liquidity account
       } else {
         addTransfer({
-          sourceAccount: destinationAccount.asset,
-          destinationAccount,
+          sourceAccountId: destinationAccount.asset.id,
+          destinationAccountId: destinationAccount.id,
           amount: destinationAmount - sourceAmount
         })
       }
@@ -287,13 +276,13 @@ export async function createTransfer(
     // Send to source liquidity account
     // Deliver from destination liquidity account
     addTransfer({
-      sourceAccount,
-      destinationAccount: sourceAccount.asset,
+      sourceAccountId: sourceAccount.id,
+      destinationAccountId: sourceAccount.asset.id,
       amount: sourceAmount
     })
     addTransfer({
-      sourceAccount: destinationAccount.asset,
-      destinationAccount,
+      sourceAccountId: destinationAccount.asset.id,
+      destinationAccountId: destinationAccount.id,
       amount: destinationAmount
     })
   }
@@ -327,19 +316,10 @@ export async function createTransfer(
       if (error) {
         return error.error
       }
-      for (const account of sourceAccounts) {
-        if (account.onDebit) {
-          const balance = await getAccountBalance(deps, account.id)
-          assert.ok(balance !== undefined)
-          await account.onDebit(balance)
-        }
-      }
-      for (const account of destinationAccounts) {
-        if (account.onCredit) {
-          const balance = await getAccountBalance(deps, account.id)
-          assert.ok(balance !== undefined)
-          await account.onCredit(balance)
-        }
+      if (destinationAccount.onCredit) {
+        const balance = await getAccountBalance(deps, destinationAccount.id)
+        assert.ok(balance !== undefined)
+        await destinationAccount.onCredit(balance)
       }
     },
     rollback: async (): Promise<void | TransferError> => {

diff --git a/packages/backend/src/connector/core/rafiki.ts b/packages/backend/src/connector/core/rafiki.ts
@@ -14,7 +14,7 @@ import {
 import { createTokenAuthMiddleware } from './middleware'
 import { RatesService } from '../../rates/service'
 import { TransferError } from '../../accounting/errors'
-import { TransferAccount, Transaction } from '../../accounting/service'
+import { LiquidityAccount, Transaction } from '../../accounting/service'
 import { AssetOptions } from '../../asset/service'
 import { AccountService } from '../../open_payments/account/service'
 import { InvoiceService } from '../../open_payments/invoice/service'
@@ -27,8 +27,8 @@ import { PeerService } from '../../peer/service'
 //   ../../open_payments/invoice/model
 //   ../../outgoing_payment/model
 //   ../../peer/model
-export interface ConnectorAccount extends TransferAccount {
-  asset: TransferAccount['asset'] & AssetOptions
+export interface ConnectorAccount extends LiquidityAccount {
+  asset: LiquidityAccount['asset'] & AssetOptions
 }
 
 export interface IncomingAccount extends ConnectorAccount {

diff --git a/packages/backend/src/tests/accountFactory.ts b/packages/backend/src/tests/accountFactory.ts
@@ -1,23 +1,30 @@
 import { v4 as uuid } from 'uuid'
 
-import {
-  AccountingService,
-  LiquidityAccount,
-  TransferAccount
-} from '../accounting/service'
+import { AccountingService, LiquidityAccount } from '../accounting/service'
 import { randomUnit } from './asset'
 
 type BuildOptions = Partial<LiquidityAccount> & {
   balance?: bigint
 }
 
+export type FactoryAccount = Omit<LiquidityAccount, 'asset'> & {
+  asset: {
+    id: string
+    unit: number
+    asset: {
+      id: string
+      unit: number
+    }
+  }
+}
+
 export class AccountFactory {
   public constructor(
     private accounts: AccountingService,
     private unitGenerator: () => number = randomUnit
   ) {}
 
-  public async build(options: BuildOptions = {}): Promise<TransferAccount> {
+  public async build(options: BuildOptions = {}): Promise<FactoryAccount> {
     const assetId = options.asset?.id || uuid()
     const unit = options.asset?.unit || this.unitGenerator()
     const asset = {