Include liquidity purchase in NewChannelIncomingPayment
#747
Conversation
modules/core/src/commonMain/kotlin/fr/acinq/lightning/channel/states/WaitForFundingSigned.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm a bit torn, because I think this is a different trade-off and it's a bit unclear whether it's really better.
I think that the mining fee for new channel creation that includes a liquidity purchase is now simpler when the LSP computed it correctly and it exactly matches the actual tx on-chain fees, but doesn't solve the inconsistency when it doesn't, so I don't know if in practice this really removes confusion or just hides a subtlety that is still likely to come up.
We're also now introducing an inconsistency between swap-in for channel creation and other types of liquidity purchases: when creating a channel from a swap-in, we don't have a separate liquidity purchase in the DB, whereas we have one in every other type of liquidity purchase...this can be "hidden" if the DB correctly includes liquidity purchases from channel creation, but this is something that requires code outside of lightning-kmp: when implementing OutgoingPaymentsDb.getInboundLiquidityPurchase(fundingTxId: TxId) we now must also look at the incoming payments DB and check if we have a channel for this fundingTxId that includes a liquidity purchase. Is that implemented in Phoenix @dpad85?
| data class ViaClose(val amount: Satoshi, override val miningFees: Satoshi, val address: String, override val txId: TxId, val isSentToDefaultAddress: Boolean, val closingType: ChannelClosingType) : StoreOutgoingPayment() | ||
| data class ViaSpliceOut(val amount: Satoshi, override val miningFee: Satoshi, val address: String, override val txId: TxId) : StoreOutgoingPayment() | ||
| data class ViaSpliceCpfp(override val miningFee: Satoshi, override val txId: TxId) : StoreOutgoingPayment() | ||
| data class ViaInboundLiquidityRequest(override val txId: TxId, override val miningFee: Satoshi, val purchase: LiquidityAds.Purchase) : StoreOutgoingPayment() |
There was a problem hiding this comment.
This is a different trade-off in terms of clarity: there is a subtlety here which is now completely hidden and should at least be explained in a comment (before your change, all the components of the total fee paid were IMO clearer because they were explicitly included in different variables).
In the miningFee field, we're including both the on-chain fees we're directly paying from the on-chain tx AND purchase.fees.miningFee, which aren't paid on-chain but rather refunded to the LSP off-chain. So miningFee should be close to the actual mining fees of the transaction, but may sometimes differ (and can be either larger or smaller than the real on-chain fees). This can be a source of confusion for users which I think must be explained.
| @@ -45,10 +45,11 @@ object Deserialization { | |||
| private fun Input.readIncomingPayment(): IncomingPayment = when (val discriminator = read()) { | |||
| 0x00 -> readBolt11IncomingPayment() | |||
| 0x01 -> readBolt12IncomingPayment() | |||
| 0x02 -> readNewChannelIncomingPayment() | |||
| // 0x02 -> do not use | |||
There was a problem hiding this comment.
Don't we need to keep deserializing this?
| // We only count the mining fees that we must refund to our peer as part of the liquidity purchase. | ||
| // If we're also contributing to the funding transaction, the mining fees we pay for our inputs and | ||
| // outputs will be recorded in the ViaNewChannel incoming payment entry below. |
No it's not, we just look at the outgoing payments. It may be possible to also look at the incoming payments and try constructing an |
Now both `OnChainIncomingPayment` and `LightningIncomingPayment` include a liquidity purchase, saving a lookup for integrators. `InboundLiquidityOutgoingPayment` is now only used for manual liquidity requests and renamed to this effect.
|
Superseded by #749. |
Since we started using the standard liquidity ads protocol to handle swap-ins, the case of a new channel creation led to two db events: a
NewChannelIncomingPaymentand aInboundLiquidityOutgoingPayment. But both from a UX and from a technical point of view, these two operations actually happen atomically. Separating them creates integration hurdles, as they need to be merged again.The first commit adds a
LiquidityAds.PurchasetoNewChannelIncomingPayment, and removes theInboundLiquidityOutgoingPaymentdb event. It also changes the meaning ofNewChannelIncomingPayment.miningFeeto be the overall mining fee (it was actually misnamed, should have beenlocalMiningFee).The second commit is just fixing consistency issues in naming.
The third commit removes
localMiningFeefromInboundLiquidityOutgoingPayment, for consistency with the above. Note that, while more pure from a technical point of view, havinglocalMiningFeea main class attribute adds mental overhead, and has no use for integrators, who typically reason in total mining fee. In Phoenix this led to niceties such as: https://github.com/ACINQ/phoenix/blob/a07c153ad4e22b042dc38fed11fab89fd1f8a4de/phoenix-shared/src/commonMain/kotlin/fr.acinq.phoenix/db/payments/InboundLiquidityQueries.kt#L100-L107