Skip to content

#2128 follow-ups - #2801

Merged
valentinewallace merged 11 commits into
lightningdevkit:mainfrom
valentinewallace:2023-12-rb-groundwork-followups
Jan 11, 2024
Merged

#2128 follow-ups#2801
valentinewallace merged 11 commits into
lightningdevkit:mainfrom
valentinewallace:2023-12-rb-groundwork-followups

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Very late follow-up from #2128. Just renames to be more descriptive.

@codecov-commenter

codecov-commenter commented Dec 18, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (9a0ed7b) 88.45%.

FilesPatch %Lines
lightning/src/ln/onion_payment.rs81.81%6 Missing ⚠️
lightning/src/ln/msgs.rs85.71%0 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs90.00%1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #2801 +/- ##
==========================================
- Coverage 88.46% 88.45% -0.01% 
==========================================
Files 114 114 Lines 91806 91810 +4 Branches 91806 91810 +4 ==========================================
- Hits 81214 81213 -1 - Misses 8112 8114 +2 - Partials 2480 2483 +3

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

shaavan
shaavan approved these changes Dec 19, 2023
View reviewed changes

@shaavanshaavan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name improvement looks great!

The new names InboundHTLCErr, onion_cltv_expiry, cltv_expiry_height, and sender_intended_htlc_amt_msat fit better in the context they are used while being clear with their functional usage.

@TheBlueMatt

TheBlueMatt commented Jan 10, 2024

Copy link
Copy Markdown
Collaborator

Needs rebase, sorry about that.

@dunxen

dunxen commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

LGTM after rebase

@valentinewallace
Remove trailing whitespace.
be134d1
@valentinewallace
Rename InboundOnionErr to InboundHTLCErr.
95a5ab9
The prior name seems to reference onion decode errors specifically, when in
fact the error contents are generic failure codes for any error that occurs
during HTLC receipt.
@valentinewallace
Rename outgoing_cltv_value internal var.
cadea5d
There is no outgoing CLTV for received HTLCs, so this new var makes more sense.
@valentinewallace
Rename InboundOnionPayload::Receive::outgoing_cltv_value
b8fa547
.. since there is no outgoing cltv for received HTLCs.
@valentinewallace
Rename InboundOnionPayload::BlindedReceive::outgoing_cltv_value.
7c3ef19
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
@valentinewallace
Rename OutboundOnionPayload::Receive::outgoing_cltv_value.
4d38ee6
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
@valentinewallace
Rename OutboundOnionPayload::BlindedReceive::outgoing_cltv_value.
aace2d3
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
@valentinewallace
Rename InboundOnionPayload::Receive::amt_msat to be more descriptive.
2d58297
@valentinewallace
Rename InboundOnionPayload::BlindedReceive::amt_msat to be more descr…
2b1c994
…iptive.
@valentinewallace
Rename OutboundOnionPayload::Receive::amt_msat to be more descriptive.
c76d74a
@valentinewallace
Rename OutboundOnionPayload::BlindedReceive::amt_msat to be more desc…
b20a9ad
…riptive.
@valentinewallace
valentinewallaceforce-pushed the 2023-12-rb-groundwork-followups branch from 9a0ed7b to b20a9adCompareJanuary 10, 2024 15:56
@valentinewallace

valentinewallace commented Jan 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Rebased.

@valentinewallacevalentinewallace mentioned this pull request Jan 10, 2024
Open
60 tasks
@valentinewallace

valentinewallace commented Jan 11, 2024

Copy link
Copy Markdown
ContributorAuthor

Gonna land this with 1 ACK since it's trivial renames and at a glance I don't see any majorly conflicting PRs. Happy to rename further in follow-up if there's post-merge review.

@valentinewallace
valentinewallace merged commit 62d52c6 into lightningdevkit:mainJan 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
+1 more reviewer
@shaavanshaavanshaavan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Easy to Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@shaavan