Account for splices in claimable balances #4029
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4029 +/- ##
==========================================
- Coverage 88.56% 88.36% -0.21%
==========================================
Files 175 177 +2
Lines 129175 131529 +2354
Branches 129175 131529 +2354
==========================================
+ Hits 114408 116226 +1818
- Misses 12246 12649 +403
- Partials 2521 2654 +133
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Discussed this one offline, needs the balance stuff to consider splices. |
wpaulino
force-pushed
the
splice-claimable-balances
branch
3 times, most recently
from
3ae60bf to
b5244ee
Compare
| @@ -928,7 +946,11 @@ impl Balance { | |||
| #[rustfmt::skip] | |||
| pub fn claimable_amount_satoshis(&self) -> u64 { | |||
| match self { | |||
| Balance::ClaimableOnChannelClose { amount_satoshis, .. }| | |||
| Balance::ClaimableOnChannelClose { balance_candidates, .. } => { | |||
| // FIXME: Should we report the balance for the currently confirmed splice | |||
There was a problem hiding this comment.
I think this is right. In practice if a splice happens you want to see your balance go down/up in accordance with where the channel is going, not where it was. We need to update the docs to describe this behavior, however.
There was a problem hiding this comment.
Let me know if the docs added are sufficient or if we should clarify a bit more elsewhere
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
wpaulino
force-pushed
the
splice-claimable-balances
branch
2 times, most recently
from
0dcabb9 to
a33a7bd
Compare
| /// The index within [`Balance::ClaimableOnChannelClose::balance_candidates`] for the | ||
| /// balance according to the current onchain state of the channel. This can be helpful when | ||
| /// wanting to determine the claimable amount when the holder commitment transaction for the | ||
| /// current funding transaction is broadcast and/or confirms. | ||
| confirmed_balance_candidate_index: usize, |
There was a problem hiding this comment.
This almost seems like an implementation detail. Should we just direct readers to claimable_amount_satoshis? Or is there another reason why they would need to use this field?
There was a problem hiding this comment.
They could use it to always report the balance according to the chain, as opposed to claimable_amount_satoshis which only reports it when one of the splice transactions has confirmed.
| pub fn claimable_amount_satoshis(&self) -> u64 { | ||
| match self { | ||
| Balance::ClaimableOnChannelClose { amount_satoshis, .. }| | ||
| Balance::ClaimableOnChannelClose { | ||
| balance_candidates, confirmed_balance_candidate_index, .. | ||
| } => { |
There was a problem hiding this comment.
Would it be worth adding another function for transaction_fee_satoshis?
There was a problem hiding this comment.
It'd be weird to only have it available for this specific variant.
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
| Balance::ClaimableOnChannelClose { | ||
| balance_candidates, confirmed_balance_candidate_index, .. | ||
| } => { | ||
| // If we have multiple candidates due to a splice, and one of the splice |
There was a problem hiding this comment.
I feel like this should be mentioned in the public API as well.
There was a problem hiding this comment.
Oh I meant include this in the docs on confirmed_balance_candidate_index since there is meaning to non-zero values that isn't explained fully.
There are two states in which a pending splice balance should be considered: 1. The channel has closed with a confirmed holder commitment transaction from a splice that did not become locked. The balance from this transaction is reported. 2. The channel is open and has multiple holder commitment transaction candidates that are valid based on the funding transaction that confirms. We want to report the pending splice balance to users while it has yet to become locked, such that they are able to see their funds have moved from their onchain wallet to the channel. We default to reporting the latest splice/RBF balance via `Balance::claimable_amount_satoshis` to mimic how an onchain wallet would behave when reporting unconfirmed balance, otherwise we report the balance for the confirmed splice transaction.
wpaulino
force-pushed
the
splice-claimable-balances
branch
from
a33a7bd to
5adf873
Compare
| Balance::ClaimableOnChannelClose { | ||
| balance_candidates, confirmed_balance_candidate_index, .. | ||
| } => { | ||
| // If we have multiple candidates due to a splice, and one of the splice |
There was a problem hiding this comment.
Oh I meant include this in the docs on confirmed_balance_candidate_index since there is meaning to non-zero values that isn't explained fully.
| /// When multiple candidates exist, the last one reflects the balance of the | ||
| /// latest splice/RBF attempt, while the first reflects the balance prior to the splice | ||
| /// occurring. | ||
| balance_candidates: Vec<HolderCommitmentTransactionBalance>, |
There was a problem hiding this comment.
Presumably this should also have docs for when things are removed from this vec.
Similar to
FundedChannel::get_available_balances, we should reflect the correct balance in the monitor based on the current state of a splice. While the channel is still open, we consider our balance to be the lowest balance across all commitment transaction candidates for the current state. Once the channel has a confirmed commitment transaction, we report the balance specific to the correspondingFundingScopebeing spent.