-
Notifications
You must be signed in to change notification settings - Fork 363
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Does it look good? |
There was a problem hiding this 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.
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. |
44063dc
to
6424436
Compare
6424436
to
5122489
Compare
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.
5122489
to
2557e6b
Compare
Addressed all your points above, ready for review. |
There was a problem hiding this 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] |
There was a problem hiding this comment.
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
/// For more accurate balances including fee information see [ChainMonitor::get_claimable_balances] | |
/// For more accurate balances including fee information see [`ChainMonitor::get_claimable_balances`] |
There was a problem hiding this 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
This PR adds
last_local_balance_msats
field. Solves #1898 .I have a few open questions:
last_local_balance_msats
, in filechannel.rs
at https://github.com/Mirebella/rust-lightning/blob/3eba0e2bbcb8547d561b175b1f19dd19565911b3/lightning/src/ln/channel.rs#L9591C1-L9591C33mod.rs
at https://github.com/Mirebella/rust-lightning/blob/3eba0e2bbcb8547d561b175b1f19dd19565911b3/lightning/src/events/mod.rs#L1538C6-L1538C44I saw that tlv fields are numbered differently(sometimes even numbers, sometimes odd numbers). I added my variable under number 11. Is that correct?