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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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 89.62%. Comparing base (f7cc40e) to head (2557e6b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
- Coverage   89.63%   89.62%   -0.01%     
==========================================
  Files         126      126              
  Lines      102399   102408       +9     
  Branches   102399   102408       +9     
==========================================
+ Hits        91785    91786       +1     
- Misses       7888     7891       +3     
- Partials     2726     2731       +5     

☔ 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.

lightning/src/events/mod.rs Outdated Show resolved Hide resolved
@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.

@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.

Introduced to show what your local balance is, when channel is closed, that way you can see how many sats you lost to rounding.
The balance may be increased by outbound pending HTLCs and transaction fees. Check [ChainMonitor::get_claimable_balances] to get more accurate balance which contains fee information.
@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

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.

3 participants