Deprecate AvailableBalances::balance_msat #3247
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dunxen
force-pushed
the
2024-08-deprecate-balancemsat
branch
from
c22796d
to
c94c933
Compare
dunxen
commented
Aug 16, 2024
dunxen
force-pushed
the
2024-08-deprecate-balancemsat
branch
from
c94c933
to
714c055
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3247 +/- ##
==========================================
+ Coverage 89.71% 89.84% +0.12%
==========================================
Files 125 125
Lines 102801 103009 +208
Branches 102801 103009 +208
==========================================
+ Hits 92226 92545 +319
+ Misses 7865 7754 -111
Partials 2710 2710 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It looks like the CI is complaining about something, probably we should relax the check for this deprecated field?
warning: build failed, waiting for other jobs to finish...
error: use of deprecated field `ln::channel_state::ChannelDetails::balance_msat`: use [`ChannelMonitor::get_claimable_balances`] instead
--> lightning/src/routing/router.rs:3605:4
|
3605 | balance_msat: 0,
| ^^^^^^^^^^^^^^^
error: use of deprecated field `ln::channel_state::ChannelDetails::balance_msat`: use [`ChannelMonitor::get_claimable_balances`] instead
--> lightning/src/routing/router.rs:8880:4
|
8880 | balance_msat: 10_000_000_000,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: could not compile `lightning` due to 7 previous errors
Yip, I'll ignore it for our usage. |
dunxen
force-pushed
the
2024-08-deprecate-balancemsat
branch
from
714c055
to
0b2fce6
Compare
TheBlueMatt
reviewed
Aug 16, 2024
lightning/src/ln/channel.rs
Outdated
| @@ -80,6 +80,7 @@ pub struct ChannelValueStat { | |||
|
|
|||
| pub struct AvailableBalances { | |||
| /// The amount that would go to us if we close the channel, ignoring any on-chain fees. | |||
| #[deprecated(since = "0.0.124", note = "use [`ChannelMonitor::get_claimable_balances`] instead")] | |||
There was a problem hiding this comment.
I don't think we need to deprecate this, its internal-only and not exported.
There was a problem hiding this comment.
I guess we can deprecate it to avoid new uses internally from creeping in causing us pain to remove this later.
TheBlueMatt
previously approved these changes
Aug 16, 2024
lightning/src/ln/channel.rs
Outdated
| @@ -80,6 +80,7 @@ pub struct ChannelValueStat { | |||
|
|
|||
| pub struct AvailableBalances { | |||
| /// The amount that would go to us if we close the channel, ignoring any on-chain fees. | |||
| #[deprecated(since = "0.0.124", note = "use [`ChannelMonitor::get_claimable_balances`] instead")] | |||
There was a problem hiding this comment.
I guess we can deprecate it to avoid new uses internally from creeping in causing us pain to remove this later.
TheBlueMatt
reviewed
Aug 16, 2024
lightning/src/ln/channel_state.rs
Outdated
| @@ -363,6 +369,10 @@ pub struct ChannelDetails { | |||
| /// This does not consider any on-chain fees. | |||
| /// | |||
| /// See also [`ChannelDetails::outbound_capacity_msat`] | |||
| #[deprecated( | |||
| since = "0.0.124", | |||
| note = "use [`ChannelMonitor::get_claimable_balances`] instead" | |||
There was a problem hiding this comment.
Should we link to ChainMonitor::get_claimable_balances instead, since that's more directly usable?
dunxen
force-pushed
the
2024-08-deprecate-balancemsat
branch
from
0b2fce6
to
f21e138
Compare
Latest changesdiff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0b15151c..731b203f 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -82,3 +82,3 @@ pub struct AvailableBalances {
/// The amount that would go to us if we close the channel, ignoring any on-chain fees.
- #[deprecated(since = "0.0.124", note = "use [`ChannelMonitor::get_claimable_balances`] instead")]
+ #[deprecated(since = "0.0.124", note = "use [`ChainMonitor::get_claimable_balances`] instead")]
pub balance_msat: u64,
diff --git a/lightning/src/ln/channel_state.rs b/lightning/src/ln/channel_state.rs
index f21822cd..7a9bbf9b 100644
--- a/lightning/src/ln/channel_state.rs
+++ b/lightning/src/ln/channel_state.rs
@@ -371,6 +371,3 @@ pub struct ChannelDetails {
/// See also [`ChannelDetails::outbound_capacity_msat`]
- #[deprecated(
- since = "0.0.124",
- note = "use [`ChannelMonitor::get_claimable_balances`] instead"
- )]
+ #[deprecated(since = "0.0.124", note = "use [`ChainMonitor::get_claimable_balances`] instead")]
pub balance_msat: u64,
diff --git a/pending_changelog/3247-deprecate-balance_msat.txt b/pending_changelog/3247-deprecate-balance_msat.txt
index 1f0eebd5..662eb3de 100644
--- a/pending_changelog/3247-deprecate-balance_msat.txt
+++ b/pending_changelog/3247-deprecate-balance_msat.txt
@@ -1 +1 @@
-* The `AvailableBalances::balance_msat` field has been deprecated in favor of `ChannelMonitor::get_claimable_balances`. `AvailableBalances::balance_msat` will be removed in an upcoming release (#3247).
+* The `AvailableBalances::balance_msat` field has been deprecated in favor of `ChainMonitor::get_claimable_balances`. `AvailableBalances::balance_msat` will be removed in an upcoming release (#3247). |
TheBlueMatt
reviewed
Aug 20, 2024
The ChannelMonitor::get_claimable_balances method provides a more straightforward approach to the balance of a channel, which satisfies most use cases. The computation of AvailableBalances::balance_msat is complex and originally had a different purpose that is not applicable anymore. We deprecate AvailableBalances::balance_msat now and will remove it in a following release. Co-authored-by: Willem Van Lint <[email protected]>
dunxen
force-pushed
the
2024-08-deprecate-balancemsat
branch
from
76b601e
to
0a1adeb
Compare
valentinewallace
approved these changes
Aug 20, 2024
TheBlueMatt
approved these changes
Aug 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ChannelMonitor::get_claimable_balancesmethod provides a more straightforward approach to the balance of a channel, which satisfies most use cases. The computation ofAvailableBalances::balance_msatis complex and originally had a different purpose that is not applicable anymore. We deprecateAvailableBalances::balance_msatnow and will remove it in a following release by #3243.See #3243 for context/motivation.