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

#2128 follow-ups #2801

Conversation

valentinewallace
Copy link
Contributor

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

@codecov-commenter
Copy link

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

Files Patch % Lines
lightning/src/ln/onion_payment.rs 81.81% 6 Missing ⚠️
lightning/src/ln/msgs.rs 85.71% 0 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs 90.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.

Copy link
Contributor

@shaavan shaavan left a comment

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
Copy link
Collaborator

Needs rebase, sorry about that.

@dunxen
Copy link
Contributor

dunxen commented Jan 10, 2024

LGTM after rebase

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.
There is no outgoing CLTV for received HTLCs, so this new var makes more sense.
.. since there is no outgoing cltv for received HTLCs.
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
There is no outgoing HTLC for received HTLCs, so rename to be more accurate.
@valentinewallace
Copy link
Contributor Author

Rebased.

@valentinewallace
Copy link
Contributor Author

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:main Jan 11, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants