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

Introduce OffersMessageFlow #3412

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented Nov 18, 2024

This PR introduces a new data structure to separate the functions and fields related to Offers Message from ChannelManager.

The change ensures a clear separation of responsibilities for BOLT12 messages, improving modularity and maintainability.

TODO:

  • Refine commit messages
  • Add detailed documentation

To decouple offers and onion message-related code from `ChannelManager`,
this commit introduces the `message_received` function in `Offer/OnionMessageHandler`.

Currently, the function focuses on handling the retry logic for `InvoiceRequest`
messages. Moving this responsibility ensures a cleaner separation of concerns
and sets the foundation for managing offers/onion messages directly within the
appropriate handler.
Since `ChannelMessageHandler`'s `message_received` function was solely
used for handling invoice request retries. This function has been
removed from `ChannelMessageHandler`, and the relevant code has been
migrated to `OffersMessageHandler`'s `message_received`.

This ensures invoice request retries are now handled in the appropriate context.
This commit introduces a new struct, `OffersMessageFlow`,
to extract all offers message-related code out of `ChannelManager`.

By moving this logic into a dedicated struct, it creates
a foundation for separating responsibilities and sets up
a base for further code restructuring in subsequent commits.
A new trait, `OffersMessageCommons`, is introduced to encapsulate
functions that are commonly used by both BOLT12-related functionality
and other parts of `ChannelManager`.

This enables a clean transition of BOLT12 code to `OffersMessageFlow`
by moving shared functions into the new trait, ensuring they remain
accessible to both `ChannelManager` and the refactored BOLT12 code.
This commit introduces the `OffersMessageHandler` implementation
for `OffersMessageFlow`, enabling direct access to offer-specific
functionality through `OffersMessageFlow`.

In subsequent commits, all other implementations of `OffersMessageHandler`
will be removed, consolidating `OffersMessageHandler` as the single
source of its implementation.
This commit introduces `OffersMessageFlow` in `functional_test`,
transitioning the source of `OffersMessageHandler` implementation
to `OffersMessageFlow`.

With this change, the `ChannelManager`'s implementation becomes
redundant and will be removed in the next commit.
With `OffersMessageFlow` now serving as the source of `OffersMessageHandler`
implementation, the implementation in `ChannelManager` is no longer needed
and has been safely removed.
- This commit introduces a new struct, `AnOffersMessageFlow`,
  which generically implements `OffersMessageFlow`.
- In subsequent commits, this struct will be utilized for
  documentation purposes.
@shaavan
Copy link
Contributor Author

shaavan commented Nov 19, 2024

Updated from pr3412.01 to pr3412.02 (diff):

Changes:

  1. Fix ci
  2. Update commit messages

@jkczyz jkczyz self-requested a review November 19, 2024 15:36
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

FYI, I reviewed this offline with @shaavan. So there will be some changes to the OffersMessageFlow trait parameterization to keep ChannelManager specifics from leaking.

return Err(Bolt12SemanticError::UnsupportedChain);
}

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

@TheBlueMatt Do you know why we need the PersistenceNotifierGuard here? This code sends an invoice for a refund (i.e., an inbound payment). IIUC, we don't persist pending_offers_messages, so it seems we don't need it.

Sign up for free to 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.

2 participants