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.

Sign up for GitHub

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

Jump to bottom

Offers without extra plugin #2976

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Offers without extra plugin #2976

wants to merge 7 commits into from

Conversation

thomash-acinq
Copy link
Member

@thomash-acinq thomash-acinq commented Jan 7, 2025 •
edited
Loading

Enable basic offer management without the need for a plugin.

Offers allow a wide range of use-cases and it would be impossible to cover everything in eclair which is why we have relied on plugins to manage offers. However most users will not need advanced features, with this PR we aim to provide the basic features that will be enough for 95% of users. Advanced users can still use a plugin to manage more complex offers.

Supported use-cases:

  • accepting donations
  • selling items without inventory management
  • compact offers using our public node id
  • private offers where our identity is protected by a blinded path

@thomash-acinq thomash-acinq changed the title Offers no plugin Offers without extra plugin Jan 7, 2025
@thomash-acinq thomash-acinq marked this pull request as draft January 7, 2025 12:48
@thomash-acinq thomash-acinq force-pushed the offers-no-plugin branch 2 times, most recently from b8611f6 to 5e9bc0e Compare January 14, 2025 10:14
@thomash-acinq thomash-acinq force-pushed the offers-no-plugin branch 5 times, most recently from 467a5a4 to 665ad0a Compare February 10, 2025 17:21
@thomash-acinq thomash-acinq force-pushed the offers-no-plugin branch 5 times, most recently from 7e0cff1 to 4b60ea1 Compare March 5, 2025 11:27
@thomash-acinq thomash-acinq marked this pull request as ready for review March 5, 2025 11:28
@thomash-acinq thomash-acinq requested a review from t-bast March 5, 2025 11:28
@thomash-acinq thomash-acinq force-pushed the offers-no-plugin branch 2 times, most recently from 89ba629 to 2220ab7 Compare March 5, 2025 12:23
t-bast
t-bast reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Mostly small architectural comments and nits at that point, the overall flow looks good!

t-bast
t-bast reviewed Mar 6, 2025
View reviewed changes
@t-bast
Clean-up and refactoring
eef2614 
This commit doesn't contain any implementation change, it only focuses
on refactoring, adding comments, logs, applying formatter suggestions
and improving tests.
t-bast
t-bast reviewed Mar 7, 2025
View reviewed changes
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, apart from the tests. I've done some refactoring and clean-up in #3030, but it should be quite straightforward (the biggest refactoring is DefaultOfferHandler, but it's purely refactoring).

eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/fixtures/MinimalNodeFixture.scala Outdated
postman: typed.ActorRef[Postman.Command],
watcher: TestProbe,
wallet: SingleKeyOnChainWallet,
bitcoinClient: TestBitcoinCoreClient) {
val nodeId = nodeParams.nodeId
val routeParams = nodeParams.routerConf.pathFindingExperimentConf.experiments.values.head.getDefaultRouteParams

val eclairImpl = new EclairImpl(Kit(nodeParams, system, watcher.ref.toTyped, paymentHandler, register, relayer, router, switchboard, paymentInitiator, TestProbe()(system).ref, TestProbe()(system).ref.toTyped, TestProbe()(system).ref.toTyped, postman, offerManager, defaultOfferHandler, wallet))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good idea, for the following reasons:

  • this forces you to use Await.Result which should be avoided unless it cannot be helped: it blocks the current thread, which means that if any of those tests fail (and we already know that the OfferPaymentSpec is flaky), it will kill our parallelism and slow down the whole test run
  • EclairImpl should be a very light shim on top of features provided by the actors that are exposed in MinimalNodeFixture, so you're not really testing the code of EclairImpl here (which should rather be tested in EclairImplSpec, when tested are needed)
  • a few actors aren't provided and are replaced by TestProbes: this will make us lose time debugging new tests that believe they can use this eclairImpl object and don't have the expected results

I think it makes more sense to implement helper functions that use standard actor programming here to:

  • create an offer (which spawns an OfferCreator like what EclairImpl does)
  • pay an offer

This should be quite straightforward to do and will keep the tests in OfferPaymentSpec easy to read?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

eclair-core/src/test/scala/fr/acinq/eclair/wire/protocol/OfferTypesSpec.scala Outdated
@@ -122,8 +122,7 @@ class OfferTypesSpec extends AnyFunSuite {
val request = InvoiceRequest(offer, 500 msat, 1, Features.empty, payerKey, Block.LivenetGenesisBlock.hash)
assert(request.isValid)
assert(request.offer == offer)
val withoutAmount = signInvoiceRequest(request.copy(records = TlvStream(request.records.records.filter { case InvoiceRequestAmount(_) => false case _ => true })), payerKey)
assert(!withoutAmount.isValid)
assertThrows[Exception](signInvoiceRequest(request.copy(records = TlvStream(request.records.records.filter { case InvoiceRequestAmount(_) => false case _ => true })), payerKey))
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit weird, it's not signInvoiceRequest that throws, but directly the creation of the InvoiceRequest object.

The change you added to provide an amount means we now immediately fail decoding those invoice_request with an exception instead of failing cleanly in the validate function...but it's fine, it makes it much simpler to use invoice requests in the codebase and nobody should be creating such invalid invoice requests (and if they do, too bad for them if we don't return a meaningful error message).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-bast t-bast t-bast left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Optech Make Me Famous!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomash-acinq @t-bast