ACINQ / phoenix

Phoenix is a self-custodial Bitcoin wallet using Lightning to send/receive payments.
https://phoenix.acinq.co
Apache License 2.0
632 stars 95 forks source link

Update sync-related hooks to support new payment types #386

Closed robbiehanson closed 12 months ago

robbiehanson commented 1 year ago

We previously had the following 3 sync-related hook methods:

expect fun didCompleteWalletPayment(/* ... */)
expect fun didDeleteWalletPayment(/* ... */)
expect fun didUpdateWalletPaymentMetadata(/* ... */)

The idea with didCompleteWalletPayment was that we didn't sync the payment to the cloud until after it had completed. For lightning payments this made sense, since generally the payment either succeeds or fails within a few seconds. So there's no use in syncing the pending payment, and then updating it 500 milliseconds later when it completes.

With the new on-chain payments, things are different. We don't want to wait for the payment to complete, because that implies waiting for a block confirmation. So I'm replacing didCompleteWalletPayment with didSaveWalletPayment, which is invoked anytime the payment is modified in the database. And now the sync code can decide what to do based on the payment type & status.

/**
 * Implement this function to execute platform specific code when a payment is saved to the database.
 * For example, on iOS this is used to enqueue the (encrypted) payment for upload to CloudKit.
 *
 * This function is invoked inside the same transaction used to add/modify the row.
 * This means any database operations performed in this function are atomic,
 * with respect to the referenced row.
 */
expect fun didSaveWalletPayment(id: WalletPaymentId, database: PaymentsDatabase)

Note that the contract guarantees the hook function is invoked within the same database transaction. (Which is required because the hook function also needs to update the database, and we want to do this in a single atomic transaction.) So I've wrapped a few function bodies in a database transaction:

// Before
fun updateFoobar(id: UUID, foobar: Boolean) {
   queries.setFoobar(
      id = id.toString(),
      foobar = foobar
   )
}

// After
fun updateFoobar(id: UUID, foobar: Boolean) {
   database.transaction {
      queries.setFoobar(
         id = id.toString(),
         foobar = foobar
      )
      didSaveWalletPayment(id, database)
   }
}

It's safe to start a transaction within a transaction:

database.transaction {
   database.transaction { // no problem
      queries.setFoobar(/* ... */)
   }
}

SqlDelight Docs give you the option to opt-out if you really want:

database.transaction {
   database.transaction(noEnclosing = true) { // throws IllegalStateException
   }
}
dpad85 commented 12 months ago

We'll need to fix the tests later on.