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.

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

Add last_local_balance_msats field #3235

Merged

Conversation

Mirebella
Copy link
Contributor

This PR adds last_local_balance_msats field. Solves #1898 .

I have a few open questions:

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.87%. Comparing base (f7cc40e) to head (5d48d58).
Report is 132 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
+ Coverage   89.64%   90.87%   +1.23%     
==========================================
  Files         126      127       +1     
  Lines      102399   116414   +14015     
  Branches   102399   116414   +14015     
==========================================
+ Hits        91793   105792   +13999     
- Misses       7881     7993     +112     
+ Partials     2725     2629      -96     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mirebella
Copy link
Contributor Author

Does it look good?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Basically looks good, just needs better docs.

@TheBlueMatt
Copy link
Collaborator

When you update the docs, please feel free to squash the commits down into one, but include more detail in the commit message - why was this change needed, what did the change do, any details about why the change was done in the way it was, etc.

@Mirebella Mirebella force-pushed the add-local-balance-msats branch from 44063dc to 6424436 Compare August 15, 2024 07:33
@Mirebella Mirebella force-pushed the add-local-balance-msats branch from 6424436 to 5122489 Compare August 28, 2024 08:25
@TheBlueMatt
Copy link
Collaborator

This basically LGTM, modulo the indentation note and the git commit message being missing. See https://cbea.ms/git-commit/ for a much, much, much longer discussion of how to write good commit messages.

@Mirebella Mirebella force-pushed the add-local-balance-msats branch from 5122489 to 2557e6b Compare September 14, 2024 08:27
@Mirebella
Copy link
Contributor Author

Addressed all your points above, ready for review.

@Mirebella Mirebella marked this pull request as ready for review September 14, 2024 08:34
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Basicaly LGTM. Can you word-wrap your commit message so that no line is longer than ~70 chars? See-also https://cbea.ms/git-commit/ for commit message writing.

/// Local balance in msats when a channel is closed.
///
/// May overstate balance by pending outbound HTLCs and transaction fees.
/// For more accurate balances including fee information see [ChainMonitor::get_claimable_balances]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, we usually tick our links so that they show up monospace

Suggested change
/// For more accurate balances including fee information see [ChainMonitor::get_claimable_balances]
/// For more accurate balances including fee information see [`ChainMonitor::get_claimable_balances`]

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

What Matt said regarding the commit, otherwise LGTM

@TheBlueMatt TheBlueMatt force-pushed the add-local-balance-msats branch from 2557e6b to 3cd6ada Compare October 7, 2024 15:18
TheBlueMatt
TheBlueMatt previously approved these changes Oct 7, 2024
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Went ahead and pushed the updates myself. They're docs-only so just gonna land:

$ git diff-tree -U2 2557e6b7e 3cd6ada3d
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index ebac6e4a9..2384a49db 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -1277,8 +1277,12 @@ pub enum Event {
 		/// Note that for instances serialized in v0.0.119 or prior this will be missing (None).
 		channel_funding_txo: Option<transaction::OutPoint>,
-		/// Local balance in msats when a channel is closed.
+		/// An upper bound on the our last local balance in msats before the channel was closed.
 		///
-		/// May overstate balance by pending outbound HTLCs and transaction fees.
-		/// For more accurate balances including fee information see [ChainMonitor::get_claimable_balances]
+		/// Will overstate our balance as it ignores pending outbound HTLCs and transaction fees.
+		///
+		/// For more accurate balances including fee information see
+		/// [`ChainMonitor::get_claimable_balances`].
+		///
+		/// This field will be `None` only for objects serialized prior to LDK 0.1.
 		last_local_balance_msats: Option<u64>,
 	},
@@ -1562,5 +1566,5 @@ impl Writeable for Event {
 			&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
 				ref counterparty_node_id, ref channel_capacity_sats, ref channel_funding_txo,
-				ref last_local_balance_msats
+				ref last_local_balance_msats,
 			} => {
 				9u8.write(writer)?;

TheBlueMatt
TheBlueMatt previously approved these changes Oct 8, 2024
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM other than the field naming mentioned.

Users commonly want to know what their balance was when a channel
was closed, which this provides in a somewhat simplified manner.

It does not consider pending HTLCs and will always overstate our
balance by transaction fees.
@TheBlueMatt
Copy link
Collaborator

Done

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGMT

@TheBlueMatt TheBlueMatt merged commit f94bf98 into lightningdevkit:main Oct 8, 2024
18 of 20 checks passed
@Mirebella
Copy link
Contributor Author

@TheBlueMatt Thank you very much

Sign up for free to 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.

5 participants