rafiki icon indicating copy to clipboard operation
rafiki copied to clipboard

Fix single phase transfer accounting

Open mkurapov opened this issue 1 year ago • 0 comments

Context

After merging in:

  • https://github.com/interledger/rafiki/pull/2564

we realized that we are unable to complete incoming payments on a receiving Rafiki, because of the way we are handling single phase transfers.

Basically, during a single phase transfer (without a timeout), we successfully send money to the underlying incoming payment liquidity account when calling accountingService.createTransfer, but since we never call transaction.post -> we never end up calling incomingPayment.onCredit(), which means, we never end up marking the incoming payment as completed, even though it has received the correct amount of funds.

What we should do (IMO), is update the accountingService to split out createTransfer into createSinglePhaseTransfer and createTwoPhaseTransfer to remove any confusion about the current behaviour of createTransfer. createSinglePhaseTransfer will return Promise<TransferError | void> while createTwoPhaseTransfer will have the same signature as createTransfer: Promise<Transaction | TransferError>, where Transaction.

Both function should call onDebit and onCredit for the respective destination and source accounts upon the successful completion of the transfer.

Additionally, I think it would make sense to have a base abstract AccountingService class, in order to simplify the current createAccountToAccountTransfer function:

export abstract class AccountingService implements AccountingService {
  constructor() {}
  abstract getBalance(id: string): Promise<bigint | undefined>
  abstract getTotalSent(id: string): Promise<bigint | undefined>
  abstract getAccountsTotalSent(ids: string[]): Promise<(bigint | undefined)[]>
  ...

  public async createTwoPhaseTransfer(
    transferArgs: TransferOptions
  ): Promise<TransferError | Transaction> {}
  public async createSinglePhaseTransfer(
    transferArgs: TransferOptions
  ): Promise<TransferError | Transaction> {}

This can be done in several PRs, the first one to make the abstract class (while keeping all of the same functionality of createTransfer), and a second one to support createTwoPhaseTransfer and createSinglePhaseTransfer

mkurapov avatar Apr 03 '24 11:04 mkurapov