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.

Sign up for GitHub

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

Jump to bottom

Rework IncomingPayment model #722

Merged
merged 9 commits into from
Dec 19, 2024
Merged

Rework IncomingPayment model #722

merged 9 commits into from
Dec 19, 2024

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Oct 23, 2024 •
edited
Loading

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

@pm47 pm47 requested a review from t-bast October 23, 2024 10:25
t-bast
t-bast reviewed Oct 24, 2024
View reviewed changes
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
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/io/Peer.kt Outdated Show resolved Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt Outdated Show resolved Hide resolved
t-bast
t-bast previously approved these changes Oct 24, 2024
View reviewed changes
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!

@pm47 pm47 force-pushed the model-rework-incoming branch from 77db5b6 to d50f720 Compare November 13, 2024 16:32
t-bast
t-bast previously approved these changes Nov 20, 2024
View reviewed changes
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 👍

@pm47 pm47 force-pushed the model-rework-incoming branch from 5450bb3 to 600c04d Compare December 11, 2024 17:46
pm47 added 8 commits December 17, 2024 17:42
@pm47
move payment id to WalletPayment
b2b760a 
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.
@pm47
rework IncomingPayment types
9bc9431 
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
rename ReceivedWith -> Received.Part
03929ad 
(Simple rename, no functional changes)

Also:
- `ReceivedWith.LightningPayment` -> `Part.Htlc`
- `ReceivedWith.AddedToFeeCredit` -> `Part.FeeCredit`
@pm47
address @t-bast comments
7da14fc
@pm47
remove deprecated PaymentReceived event
2b08260
@pm47
improve LegacyPayToOpenIncomingPayment
f09bcb8
@pm47
add helper methods to update child classes
d3123af
@pm47
remove the LightningIncomingPayment.Received class
bad4ec3 
Instead, the `parts` are a direct member of `LightningIncomingPayment`.
It makes the object simpler to manipulate and update. Each of them has
a `receivedAt` timestamp, offering more precision.
@pm47 pm47 force-pushed the model-rework-incoming branch from a110612 to bad4ec3 Compare December 17, 2024 16:55
@pm47 pm47 marked this pull request as ready for review December 17, 2024 17:31
This was referenced Dec 17, 2024
Merged
Merged
t-bast
t-bast reviewed Dec 18, 2024
View reviewed changes
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.

A few nits, otherwise LGTM 👍

modules/core/src/commonMain/kotlin/fr/acinq/lightning/db/PaymentsDb.kt Outdated Show resolved Hide resolved
...s/core/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt Outdated Show resolved Hide resolved
...s/core/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt Show resolved Hide resolved
@pm47 pm47 requested a review from t-bast December 19, 2024 14:08
@pm47 pm47 merged commit 6aab996 into master Dec 19, 2024
2 checks passed
@pm47 pm47 deleted the model-rework-incoming branch December 19, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpad85 dpad85 dpad85 left review comments

@t-bast t-bast t-bast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pm47 @dpad85 @t-bast