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.

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

Fix single phase transfer accounting #2629

Open
mkurapov opened this issue Apr 3, 2024 · 0 comments
Open

Fix single phase transfer accounting #2629

mkurapov opened this issue Apr 3, 2024 · 0 comments
Labels
pkg: backend Changes in the backend package.

Comments

@mkurapov
Copy link
Contributor

mkurapov commented Apr 3, 2024

Context

After merging in:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package.
Projects
Status: Backlog
Development

No branches or pull requests

2 participants