offer::Quantity::One is incompatible with quantity: Some(1)

[tnull]

This is a bit confusing: The Quantity variant is named One, but pay_for_offer fails to send the invoice request with an UnexpectedQuantity error if the quantity argument is Some(1).

Seems the source of this is expects_quantity in offer.rs returning false for the One variant.

Discovered by @slanesuke over at lightningdevkit/ldk-node#327

(cc @jkczyz)

[tnull]

Thinking about it, I also wonder why the default isn't Unbounded? Why do we default to this "quantity-is-unset-implictly-meaning-its-one" variant?

[jkczyz]

I believe we are compliant with the spec:

  - if it can supply more than one item for a single invoice:
    - if the maximum quantity is known:
      - MUST set that maximum in `offer_quantity_max`.
      - MUST NOT set `offer_quantity_max` to 0.
    - otherwise:
      - MUST set `offer_quantity_max` to 0.
  - otherwise:
    - MUST NOT set `offer_quantity_max`.

Although offer_qunatity_max is still allowed to be 1.

`offer_quantity_max` is allowed to be 1, which seems useless, but
useful in a system which bases it on available stock.  It would be
painful to have to special-case the "only one left" offer generation.

Then when creating an invoice request:

    - if `offer_quantity_max` is present:
      - MUST set `invreq_quantity` to greater than zero.
      - if `offer_quantity_max` is non-zero:
        - MUST set `invreq_quantity` less than or equal to `offer_quantity_max`.
     - otherwise:
      - MUST NOT set `invreq_quantity`

Using None for the common case (i.e., one item) is likely to save bytes in an offer QR code.

[tnull]

I believe we are compliant with the spec:

Hmm, that might be the case, but it's a very confusing (and error-prone) API if Quantity::Bounded(1) and Quantity::One are two completely separate, incompatible types, even though semantically they both express the same thing?

How would the user know when to use which, or rather, how would they know never to set quantity to Some(1) if the offer signals a Quantity::One being available for purchase, in particular since on pay_for_offer it's given as quantity: Option<u64>, not as a Quantity?

Using None for the common case (i.e., one item) is likely to save bytes in an offer QR code.

If we want to keep this behavior, should our API enforce this rather than just briefly mentioning it in the docs, e.g., have Quantity be a (wrapping?) struct with private field for which we provide constructors that would a) make sure we always use QuantityType::One over QuantityType::Bounded(1) and b) Return an error if the given quantity exceeds the max quantity or is 0, which would allow us to avoid NonZeroU64 in the API?

[jkczyz]

Hmm, that might be the case, but it's a very confusing (and error-prone) API if Quantity::Bounded(1) and Quantity::One are two completely separate, incompatible types, even though semantically they both express the same thing?

How would the user know when to use which, or rather, how would they know never to set quantity to Some(1) if the offer signals a Quantity::One being available for purchase, in particular since on pay_for_offer it's given as quantity: Option<u64>, not as a Quantity?

Currently, they need to call Offer::supported_quantity though as suppose we could do that for them in pay_for_offer. But they would pass a u64 not Quantity if we remove the Option.

Using None for the common case (i.e., one item) is likely to save bytes in an offer QR code.

If we want to keep this behavior, should our API enforce this rather than just briefly mentioning it in the docs, e.g., have Quantity be a (wrapping?) struct with private field for which we provide constructors that would a) make sure we always use QuantityType::One over QuantityType::Bounded(1) and b) Return an error if the given quantity exceeds the max quantity or is 0, which would allow us to avoid NonZeroU64 in the API?

Are you talking about the sending or the receiving API? Quantity only shows up in the receiving side. I'm not sure if there is a reason to change this if we can add a check in the sender side as mentioned above. Maybe I don't quite understand what you are proposing for each side.

[tnull]

Currently, they need to call Offer::supported_quantity though as suppose we could do that for them in pay_for_offer. But they would pass a u64 not Quantity if we remove the Option.

Right, but that would enforce giving a quantity which doesn't make sense in certain contexts?

Are you talking about the sending or the receiving API? Quantity only shows up in the receiving side. I'm not sure if there is a reason to change this if we can add a check in the sender side as mentioned above. Maybe I don't quite understand what you are proposing for each side.

In this case I was talking about the receiving side. I simply find it confusing that the use of Quantity::Bounded(1) is discouraged in the docs, but we don't actually enforce it in the API. I.e., a receiver might still just set Quantity::Bounded(1) and a payer not giving a quantity would error. Or, the receiver would leave it Quantity::One and the payer would error when trying to pay_for_offer with quantity Some(1), even though semantically they are essentially the same (no?). So we could consider building an API that would enforce always using Quantity::One if the receiver tries to set a quantity of 1, essentially.

[jkczyz]

Right, but that would enforce giving a quantity which doesn't make sense in certain contexts?

True, though then I'd argue we should keep it as an Option and do the right thing inside pay_for_offer when possible.

In this case I was talking about the receiving side. I simply find it confusing that the use of Quantity::Bounded(1) is discouraged in the docs, but we don't actually enforce it in the API. I.e., a receiver might still just set Quantity::Bounded(1) and a payer not giving a quantity would error. Or, the receiver would leave it Quantity::One and the payer would error when trying to pay_for_offer with quantity Some(1), even though semantically they are essentially the same (no?). So we could consider building an API that would enforce always using Quantity::One if the receiver tries to set a quantity of 1, essentially.

We need to be able to deal with offers not created by LDK. This requires creating an InvoiceRequest with the Offer's original representation. We lose that if we translate Quantity::Bounded(1) to Quantity::One¹. And when examining the Offer we wouldn't give an accurate representation.

Instead, if the user doesn't specify a quantity when calling pay_for_offer, we could possibly translate it to Some(1) if Offer::supported_quantity is Quantity::Bounded(1).

Footnotes

1. We do take the original byes when creating the InvoiceRequest, but the as_tlv_stream results will be wrong. So maybe it wouldn't be a problem but possibly could be a source of confusion / buggy behavior if the code changes. ↩

[tnull]

Instead, if the user doesn't specify a quantity when calling pay_for_offer, we could possibly translate it to Some(1) if Offer::supported_quantity is Quantity::Bounded(1).

Hmm, then I also wonder if we should supporting unsetting Some(1) if supported_quantity is Quantity::One, i.e., translate both ways in pay_for_offer?

Or maybe translating is error prone depending on how the user expects to use the offers API, and we're better of with the current API which I find confusing but at least would rather fail than introducing unintended effects?

[jkczyz]

Instead, if the user doesn't specify a quantity when calling pay_for_offer, we could possibly translate it to Some(1) if Offer::supported_quantity is Quantity::Bounded(1).

Hmm, then I also wonder if we should supporting unsetting Some(1) if supported_quantity is Quantity::One, i.e., translate both ways in pay_for_offer?

Yes, we would need to otherwise InvoiceRequestBuilder::quantity would fail. Otherwise, we wouldn't conform to the spec.

Or maybe translating is error prone depending on how the user expects to use the offers API, and we're better of with the current API which I find confusing but at least would rather fail than introducing unintended effects?

I'm fine with either. We can make it less error-prone at the ChannelManager level, IMO.