Uh oh!
There was an error while loading. Please reload this page.
Correct variable names in ChannelMonitor and DRY tests - #1494
Merged
TheBlueMatt merged 4 commits intoMay 26, 2022
Conversation
Several fields used in tracking on-chain HTLC outputs were named `input_idx` despite referring to the output index in the commitment transaction. Here they are all renamed `commitment_tx_output_idx` for clarity.
TheBlueMatt
commented
May 25, 2022
CollaboratorAuthor
Assigning 108 as #1495 depends on it. |
arik-so
previously approved these changes
May 25, 2022
| channel_value_satoshis: u64, | ||
| // first is the idx of the first of the two revocation points | ||
| their_cur_revocation_points: Option<(u64, PublicKey, Option<PublicKey>)>, | ||
| their_cur_per_commitment_points: Option<(u64, PublicKey, Option<PublicKey>)>, |
Contributor
There was a problem hiding this comment.
I feel like variable names are free, so we could spell out "current" or "latest"
Contributor
There was a problem hiding this comment.
nope, just marginally easier on the reader
CollaboratorAuthor
There was a problem hiding this comment.
Eh, I'm trying to avoid writing out too long variable names so that rustfmt doesn't break things into more lines...unless its critical I'd prefer not to.
| } | ||
| let preimage = if htlc.offered { if let Some(p) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; | ||
| if preimage.is_some() || !htlc.offered { | ||
| let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput(CounterpartyOfferedHTLCOutput::build(*revocation_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, preimage.unwrap(), htlc.clone())) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput(CounterpartyReceivedHTLCOutput::build(*revocation_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, htlc.clone())) }; |
Contributor
There was a problem hiding this comment.
lol how was this ever one line
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMattforce-pushed
the
2022-05-mon-cleanups-renames
branch
from
May 25, 2022 23:09
190a6b9 to
1f69c96Comparecodecov-commenter
commented
May 25, 2022
Codecov Report
@@ Coverage Diff @@## main #1494 +/- ##
==========================================
- Coverage 90.93% 90.93% -0.01%
==========================================
Files 77 77 Lines 42394 42359 -35 Branches 42394 42359 -35 ==========================================
- Hits 38553 38521 -32 + Misses 3841 3838 -3
Continue to review full report at Codecov.
|
In `HTLCUpdate` and `OnchainEvent` tracking, we store the HTLC value (rounded down to whole satoshis). This is somewhat confusingly referred to as the `onchain_value_satoshis` even though it refers to the commitment transaction output value, not the value available on chain (which may have been reduced by an HTLC-Timeout/HTLC-Success transaction).
The `ChannelMonitor` had a field for the counterparty's `cur_revocation_points`. Somewhat confusingly, this actually stored the counterparty's *per-commitment* points, not the (derived) revocation points. Here we correct this by simply renaming the references as appropriate. Note the update in `channel.rs` makes the variable names align correctly.
TheBlueMattforce-pushed
the
2022-05-mon-cleanups-renames
branch
from
May 26, 2022 00:50
1f69c96 to
d443b79CompareTheBlueMatt
commented
May 26, 2022
CollaboratorAuthor
Squashed without changes: |
wpaulino
approved these changes
May 26, 2022
tnull
approved these changes
May 26, 2022
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
While working on #1466, i noticed a few things in
ChannelMonitorare confusingly- (or mis-)named, plusmonitor_tests.rshad some obvious DRY'ability. This gets those out of the way so that changes to address #1466 are simpler to read.