feat: skip fee probe for flagged destinations

[vindard]

Description

This PR fixes an issue where some services will always fail the fee probe whether there is a route or not (e.g. Muun). This is an issue because the attempt is logged in lnd's mission-control and it blocks any real payments attempts that are tried soon after the probe.

This PR allows us to configure flagged destinations that will fail and to skip the probe and simply return a max fee for these routes.

Related issues:

• payment failure with wallet that open channels on the fly #32

[nicolasburtey]

on muun we should not skip the final hop, but the penultimate (one before last) node.

because muun uses a private channel for the last hop.

ie: the last hop will be an unpublished nodes, but if you look at the route hints encoded in the invoice, they would both be linked between muun and this unpublished nodes.

is that something you taken into consideration in this PR?

[vindard]

Yup, it's captured in the code logic and in the test (test has an actual Muun invoice)

[bodymindarts]

Don't like that this is an integration test. The logic doesn't depend on user input - should be a unit test.

[vindard]

I added domain + unit test changes (1ba3743), but I think this integration test is still useful to confirm that the lnd service method isn't actually being called when we're using domain calculated states to pull together the final use-case.

[bodymindarts]

I think this logic belongs inside the builder - it can take the pub keys to skip as config:

type LightningPaymentFlowBuilderConfig = {
  localNodeIds: Pubkey[]
  skipProbeKeys: Pubkey[]
  usdFromBtcMidPriceFn: UsdFromBtcMidPriceFn
  btcFromUsdMidPriceFn: BtcFromUsdMidPriceFn
}

or something like that.

This step:

type LPFBWithConversion<S extends WalletCurrency, R extends WalletCurrency> = {
  withRoute({
    pubkey,
    rawRoute,
  }: {
    pubkey: Pubkey
    rawRoute: RawRoute
  }): Promise<PaymentFlow<S, R> | ValidationError | DealerPriceServiceError>
  withoutRoute(): Promise<PaymentFlow<S, R> | ValidationError | DealerPriceServiceError>

  btcPaymentAmount(): Promise<BtcPaymentAmount | DealerPriceServiceError>
  usdPaymentAmount(): Promise<UsdPaymentAmount | DealerPriceServiceError>

  isIntraLedger(): Promise<boolean | DealerPriceServiceError>
}

Already has some query methods - could also add another one shouldSkipProbe?

There is no way this logic should be integration tested.

[vindard]

Done: 1ba3743

[dolcalmi]

why not just an array?

[vindard]

I thought it would be useful to have some context if we had to add any other services.

[nicolasburtey]

I would agree with @dolcalmi that just a series of keys would be enough.

you could have the name of the service in comments, or it's easy enough to fetch it otherwise.

ie:

skipFeeProbe:
  - "038f8f113c580048d847d6949371726653e02b928196bad310e3eda39ff61723f6" # muun

[dolcalmi]

this should have a custom message. Sending Unknown error occurred is not good for this case

[vindard]

This is just to satisfy the type checker. This error never gets returned to the user right now. It's just used in a conditional to determine if to call withoutRoute or withRoute on the builder.

[bodymindarts]

All looks good except I don't understand the extra config key... flaggedPubkeys is also config isn't it?

[vindard]

All looks good except I don't understand the extra config key... flaggedPubkeys is also config isn't it?

Yup I noticed this literally after I pushed the last commit. Changed now.

Questions addressed