Clarify handling of use_trampoline in sdk-common structs

[ok300]

There is one struct in particular that uses this behind a feature flag:

breez-sdk-greenlight/libs/sdk-common/src/lnurl/specs/pay.rs

Lines 128 to 147 in 1298055

	/// Represents a LNURL-pay request.
	#[derive(Clone, Debug, Serialize, Deserialize)]
	pub struct LnUrlPayRequest {
	/// The [LnUrlPayRequestData] returned by [crate::input_parser::parse]
	pub data: LnUrlPayRequestData,
	/// The amount in millisatoshis for this payment
	pub amount_msat: u64,
	#[cfg(not(feature = "liquid"))] // Only available for the Greenlight SDK
	/// Trampoline payments outsource pathfinding to the LSP. Trampoline payments can improve
	/// payment performance, but are generally more expensive in terms of fees and they
	/// compromise on privacy.
	pub use_trampoline: bool,
	/// An optional comment for this payment
	pub comment: Option<String>,
	/// The external label or identifier of the [Payment]
	pub payment_label: Option<String>,
	/// Validates that, if there is a URL success action, the URL domain matches
	/// the LNURL callback domain. Defaults to `true`
	pub validate_success_action_url: Option<bool>,
	}

Question:

• Should sdk-common remove the liquid feature flag and instead only contain truly common structs and code?
  • If so, should there be a separate LnUrlPayRequestWithTranpoline (or similar) in the GL SDK repo?
• Alternatively, is the current approach ok and we should keep it?

Let's discuss below.

[JssDWt]

Is it possible to have LnUrlPayRequest in sdk greenlight and LnUrlPayRequest in sdk liquid, but they're different structs? That would be my preferred option.

[ok300]

IMO the feature flag in sdk-common is overkill. We should instead place only common code in sdk-common. What's implementation-specific should live in the implementation-specific repo (for example a new struct with use_trampoline in the GL SDK repo).

[ok300]

Is it possible to have LnUrlPayRequest in sdk greenlight and LnUrlPayRequest in sdk liquid, but they're different structs? That would be my preferred option.

I don't think so, as lnurl_pay lives in sdk-common and it has to know which request struct to reference.

[JssDWt]

I don't think so, as lnurl_pay lives in sdk-common and it has to know which request struct to reference.

Is it? I can't find it.

[JssDWt]

There might be another option here too.
use_trampoline could be a configuration-time setting instead. That would mean it applies to all payments. Then this problem would no longer exist, because the field would not be necessary on a specific payment. What do you think @roeierez?

[ok300]

You're right, lnurl_pay is not in sdk-common but instead implemented (slightly differently) in Liquid and GL SDK repos.

Then your idea could well work. The GL SDK repo lnurl_pay could reference its own request struct with use_trampoline, the Liquid one could reference the sdk-common request struct without use_trampoline.

[roeierez]

I think the whole problem is that the udl interface has leaked into the sdk-common. We should leave there only "business-logic" or business models.
So I don't see any issue in creating two different structures and two different udl definitions of the two projects.
If I am wrong please point me to any issue you find.

[JssDWt]

I think the whole problem is that the udl interface has leaked into the sdk-common.

With udl definitions you mean the rust structs that are also defined in the udl? (asking because the udl file is still in sdk-bindings)

[erdemyerebasmaz]

Despite being more error-prone than your suggestions, the current approach of using feature flags—#[cfg(feature = "liquid")] and #[cfg(not(feature = "liquid"))]—to include or exclude fields from shared structs is imo adequate.

I am not sure if this is worth doubling the work over, we can add validation tests for sdk-common w/ & w/o liquid feature on the CI to catch any issues.

I'm open to any solutions that does not double the work managing these structs.

[roeierez]

I think the whole problem is that the udl interface has leaked into the sdk-common.

With udl definitions you mean the rust structs that are also defined in the udl? (asking because the udl file is still in sdk-bindings)

I am referring to the fact that the LnUrlPayRequest is defined in sdk common but is not used there anywhere, it is only used in sdk-core and liquid project. So in fact we can create for both of these referring projects their own LnUrlPayRequest. Not only that the LnUrlPayRequest is the interface implementation (udl file) so no reason both will share the same interface (or at least no reason to require that). Both interfaces have different motivation to change (different implementations).