Skip to content

Quick #3010 followups - #3311

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-09-3010-followups
Sep 30, 2024
Merged

Quick #3010 followups#3311
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-09-3010-followups

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Few minor things I noticed in the second-pass review.

@vincenzopalazzovincenzopalazzo 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.

LGTM

Comment threadlightning/src/ln/offers_tests.rs Outdated
@arik-so

Copy link
Copy Markdown
Contributor

LGTM less the typo, though

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

LGTM mod typo! 🚀

`creates_and_pays_for_offer_with_retry` intends to check that we
re-send a BOLT 12 `invoice_request` in response to a
`message_received` call, but doesn't actually test that there were
no messages in the outbound buffer after the initial send, which we
do here.
While `message_received` purports to be called on every message,
prior to the message, doing so on `Init` messages means we have to
call `message_received` while holding the per-peer mutex, which
can cause some lock contention.
Instead, here, we call `message_received` after processing `Init`
messages (which is probably more useful anyway - the peer isn't
really "connected" until we've processed the `Init` messages),
allowing us to call it unlocked.
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Fixed, sorry.

@codecov

codecovBot commented Sep 30, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.17%. Comparing base (1059f5f) to head (d156b2e).
Report is 100 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #3311 +/- ##
==========================================
+ Coverage 89.63% 91.17% +1.53% 
==========================================
Files 126 126 Lines 102383 114710 +12327 Branches 102383 114710 +12327 ==========================================
+ Hits 91776 104588 +12812 + Misses 7877 7560 -317 + Partials 2730 2562 -168 
FlagCoverage Δ
91.17% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@TheBlueMatt

TheBlueMatt commented Sep 30, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Pretty simple and three people have looked at it so just landing.

@TheBlueMatt
TheBlueMatt merged commit c7627df into lightningdevkit:mainSep 30, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@arik-so@vincenzopalazzo@shaavan