Correct variable names in ChannelMonitor and DRY tests
#1494
Conversation
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
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.
|
Assigning 108 as #1495 depends on it. |
arik-so
previously approved these changes
May 25, 2022
| @@ -620,7 +620,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> { | |||
| funding_redeemscript: Script, | |||
| 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>)>, | |||
There was a problem hiding this comment.
I feel like variable names are free, so we could spell out "current" or "latest"
There was a problem hiding this comment.
Is it ambiguous?
There was a problem hiding this comment.
nope, just marginally easier on the reader
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.
| @@ -2142,7 +2143,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |||
| } | |||
| 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())) }; | |||
There was a problem hiding this comment.
lol how was this ever one line
wpaulino
reviewed
May 25, 2022
TheBlueMatt
force-pushed
the
2022-05-mon-cleanups-renames
branch
from
190a6b9 to
1f69c96
Compare
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.
TheBlueMatt
force-pushed
the
2022-05-mon-cleanups-renames
branch
from
1f69c96 to
d443b79
Compare
|
Squashed without changes: |
wpaulino
approved these changes
May 26, 2022
tnull
approved these changes
May 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.