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

Rework IncomingPayment model #722

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Rework IncomingPayment model #722

wants to merge 5 commits into from

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Oct 23, 2024

We move from a flat model with a single IncomingPayment class and a combination of Origin + ReceivedWith parts to a hierarchical model.

This new model:

  • makes incoming/outgoing databases symmetrical
  • removes impossible combinations allowed in the previous model (e.g. an on-chain origin with a fee-credit part)
  • removes hacks required for the handling of on-chain deposits, which were shoe-horned in what was originally a lightning-only model

Before:

Origin
   |
   `--- Invoice
   |
   `--- Offer
   |
   `--- SwapIn
   |
   `--- OnChain

ReceivedWith
      |
      `--- LightningPayment
      |
      `--- AddedToFeeCredit
      |
      `--- OnChainIncomingPayment
                    |
                    `--- NewChannel
                    |
                    `--- SpliceIn

After:

IncomingPayment
      |
      `--- LightningIncomingPayment
      |             |
      |             `--- Bolt11IncomingPayment
      |             |
      |             `--- Bolt12IncomingPayment
      |
      `--- OnChainIncomingPayment
                    |
                    `--- NewChannelIncomingPayment
                    |
                    `--- SpliceInIncomingPayment
                    |
                    `--- LegacySwapInIncomingPayment
                    |
                    `--- LegacyPayToOpenIncomingPayment

LightningIncomingPayment.Part
      |
      `--- Htlc
      |
      `--- FeeCredit

The handling of backward compatible data is tricky, especially for legacy pay-to-open/pay-to-splice, which can be a mix of lightning parts and on-chain parts, and either Bolt11 or Bolt12. Note that Legacy* classes are not used at all within lightning-kmp, they are just meant to handle pre-existing data in the database.

payment old model new model
Plain Lightning Bolt11 Origin=Invoice, ReceivedWith=LightningPayment Bolt11IncomingPayment
Plain Lightning Bolt12 Origin=Offer, ReceivedWith=LightningPayment Bolt12IncomingPayment
Pre-otf pay-to-open Bolt11 Origin=Invoice, ReceivedWith=NewChannel LegacyPayToOpenIncomingPayment
Pre-otf pay-to-splice Bolt11 Origin=Invoice, ReceivedWith=SpliceIn LegacyPayToOpenIncomingPayment
Pre-otf pay-to-open Bolt12 Origin=Offer, ReceivedWith=NewChannel LegacyPayToOpenIncomingPayment
Pre-otf pay-to-splice Bolt12 Origin=Offer, ReceivedWith=SpliceIn LegacyPayToOpenIncomingPayment
Legacy trusted swap-in Origin=SwapIn, ReceivedWith=NewChannel LegacySwapInIncomingPayment
Swap-in potentiam open Origin=OnChain, ReceivedWith=NewChannel NewChannelIncomingPayment
Swap-in potentiam splice Origin=OnChain, ReceivedWith=SpliceIn SpliceInIncomingPayment

This allows for a unified identifier for all payments that affect the
balance.

For `IncomingPayment`, the `payment_hash` is currently abusively used as
a primary key, which is hacky in the context of swap-ins. To minimize
integration effort, the payment id simply derived from that existing
key, with no other changes.
We move from a flat model with a single `IncomingPayment` class and a
combination of `Origin` + `ReceivedWith` parts to a hierarchical model.

This new model:
- makes incoming/outgoing databases symmetrical
- removes impossible combinations allowed in the previous model (e.g. an
  on-chain origin with a fee-credit part)
- removes hacks required for the handling of on-chain deposits, which
  were shoe-horned in what was originally a lightning-only model

Before:
```
Origin
   |
   `--- Invoice
   |
   `--- Offer
   |
   `--- SwapIn
   |
   `--- OnChain

ReceivedWith
      |
      `--- LightningPayment
      |
      `--- AddedToFeeCredit
      |
      `--- OnChainIncomingPayment
                    |
                    `--- NewChannel
                    |
                    `--- SpliceIn
```

After:
```
IncomingPayment
      |
      `--- LightningIncomingPayment
      |             |
      |             `--- Bolt11IncomingPayment
      |             |
      |             `--- Bolt12IncomingPayment
      |
      `--- OnChainIncomingPayment
                    |
                    `--- NewChannelIncomingPayment
                    |
                    `--- SpliceInIncomingPayment
                    |
                    `--- LegacySwapInIncomingPayment
                    |
                    `--- LegacyPayToOpenIncomingPayment

LightningIncomingPayment.Part
      |
      `--- LightningPayment
      |
      `--- AddedToFeeCredit
```

The handling of backward compatible data is tricky, especially for
legacy pay-to-open/pay-to-splice, which can be a mix of lightning parts
and on-chain parts, and either Bolt11 or Bolt12. Note that `Legacy*`
classes are not used at all within `lightning-kmp`, they are just meant
to handle pre-existing data in the database.
@pm47 pm47 requested a review from t-bast October 23, 2024 10:25
(Simple rename, no functional changes)

Also:
- `ReceivedWith.LightningPayment` -> `Part.Htlc`
- `ReceivedWith.AddedToFeeCredit` -> `Part.FeeCredit`
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, this is a good clean-up! It's simpler than I thought, the hard part is probably the DB part on the Phoenix side?

I don't see any issue on the lightning-kmp side, everything is much clearer now.

src/commonMain/kotlin/fr/acinq/lightning/db/PaymentsDb.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/db/PaymentsDb.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/db/PaymentsDb.kt Outdated Show resolved Hide resolved
@@ -139,7 +139,7 @@ data class SendOnTheFlyFundingMessage(val message: OnTheFlyFundingMessage) : Pee
sealed class PeerEvent

@Deprecated("Replaced by NodeEvents", replaceWith = ReplaceWith("PaymentEvents.PaymentReceived", "fr.acinq.lightning.PaymentEvents"))
data class PaymentReceived(val incomingPayment: IncomingPayment, val received: IncomingPayment.Received) : PeerEvent()
data class PaymentReceived(val incomingPayment: LightningIncomingPayment, val received: LightningIncomingPayment.Received) : PeerEvent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this one entirely? It has been deprecated for a while.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @dpad85

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 77db5b6.

t-bast
t-bast previously approved these changes Oct 24, 2024
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as far as backwards-compatibility doesn't create any issue for Phoenix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants