Skip to content

Deprecate AvailableBalances::balance_msat - #3247

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
dunxen:2024-08-deprecate-balancemsat
Aug 20, 2024
Merged

Deprecate AvailableBalances::balance_msat#3247
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
dunxen:2024-08-deprecate-balancemsat

Conversation

@dunxen

Copy link
Copy Markdown
Contributor

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 by #3243.

See #3243 for context/motivation.

@dunxen
dunxenforce-pushed the 2024-08-deprecate-balancemsat branch from c22796d to c94c933CompareAugust 16, 2024 09:44
Comment threadlightning/src/ln/channel_state.rs Outdated
@dunxen
dunxen marked this pull request as ready for review August 16, 2024 09:46
@dunxen
dunxenforce-pushed the 2024-08-deprecate-balancemsat branch from c94c933 to 714c055CompareAugust 16, 2024 09:47
@codecov

codecovBot commented Aug 16, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.84%. Comparing base (680d399) to head (0a1adeb).
Report is 9 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/channel_state.rs98.98%0 Missing and 1 partial ⚠️
lightning/src/routing/router.rs50.00%1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@vincenzopalazzovincenzopalazzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@dunxen

Copy link
Copy Markdown
ContributorAuthor

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
dunxenforce-pushed the 2024-08-deprecate-balancemsat branch from 714c055 to 0b2fce6CompareAugust 16, 2024 13:54
Comment threadlightning/src/ln/channel.rs Outdated

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")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to deprecate this, its internal-only and not exported.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess we can deprecate it to avoid new uses internally from creeping in causing us pain to remove this later.

TheBlueMatt
TheBlueMatt previously approved these changes Aug 16, 2024
Comment threadlightning/src/ln/channel.rs Outdated

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")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess we can deprecate it to avoid new uses internally from creeping in causing us pain to remove this later.

Comment threadlightning/src/ln/channel_state.rs Outdated
/// See also [`ChannelDetails::outbound_capacity_msat`]
#[deprecated(
since = "0.0.124",
note = "use [`ChannelMonitor::get_claimable_balances`] instead"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we link to ChainMonitor::get_claimable_balances instead, since that's more directly usable?

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM after Matt's comment

@dunxen

Copy link
Copy Markdown
ContributorAuthor
Latest changes
diff --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).

Comment threadlightning/src/ln/channel_state.rs
@TheBlueMattTheBlueMatt added this to the 0.0.124 milestone Aug 20, 2024
dunxenand others added 2 commits August 20, 2024 17:08
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 <noreply@wvanlint.dev>
@dunxen
dunxenforce-pushed the 2024-08-deprecate-balancemsat branch from 76b601e to 0a1adebCompareAugust 20, 2024 15:25
@TheBlueMatt
TheBlueMatt merged commit bbfa15e into lightningdevkit:mainAug 20, 2024
@dunxen
dunxen deleted the 2024-08-deprecate-balancemsat branch August 21, 2024 13:09
Sign up for freeto 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.

4 participants

@dunxen@TheBlueMatt@valentinewallace@vincenzopalazzo