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

[feature]: create RFQ wire messages #780

Closed
ffranr opened this issue Feb 1, 2024 · 7 comments
Closed

[feature]: create RFQ wire messages #780

ffranr opened this issue Feb 1, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request rfq
Milestone

Comments

@ffranr
Copy link
Contributor

ffranr commented Feb 1, 2024

Parent issue: #683

In this issue we finalise the RFQ wire messages golang package. Here's what we currently have in the BLIP:

Request For Quote (tap_rfq)

When a receiver wishes to receive N units of TAP asset ID asset_id, a new
p2p message tap_rfq is sent with the following structure:

  1. type: ?? (tap_rfq)
  2. data:
    • [32*byte:rfq_id]
    • [32*byte:asset_id]
    • [BigSize:asset_amt]
    • [BigSize:suggested_rate_tick]

where:

  • rfq_id is a randomly generate 32-byte value to uniquely identify this RFQ
    request
  • asset_id is the asset ID of the asset the receiver intends to receive
  • asset_amt is the amount of units of said asset
  • suggested_rate_tick is the internal unit used for asset conversions. A tick
    is 1/10000th of a currency unit. It gives us up to 4 decimal places of
    precision (0.0001 or 0.01% or 1 bps). As an example, if the BTC/USD rate was
    $61,234.95, then we multiply that by 10,000 to arrive at the usd_rate_tick:
    $61,234.95 * 10000 = 612,349,500. To convert back to our normal rate, we
    decide by 10,000 to arrive back at $61,234.95.

Given valid rfq_id, we then define an tap_rfq_scid by taking the last 8
bytes of the rfq_id and interpreting them as a 64-bit integer.

Accepting Quotes (tap_rfq_accept)

If it can, then it should send tap_rfq_accept that returns the quote amount
the edge node is willing to observe to move N units of asset asset_id:

  1. type: ?? (tap_req_accept)
  2. data:
    • [32*byte:rfq_id]
    • [BigSize:`accepted_rate_tick]
    • [BigSize:expiry_seconds]
    • [64*byte:rfq_sig]

TODO(roasbeef): tlv err where?

where:

  • rfq_id matches the existing rfq_id of a set tap_rfq

  • accepted_rate_tick is the proposed rate for the volume unit expressed in
    the internal unit of a tick.

  • expiry_seconds is the amount of seconds to use for the expiry of both the
    quote and the invoice

  • rfq_sig is a signature over the serialized contents of the message

Rejecting Quotes (tap_rfq_reject)

In the event that an edge node is unable to satisfy a quote request, then they
should send tap_rfq_reject, identifying the rejected quote ID. A quote might
be rejected if the channel cannot accommodate the proposed volume, or if the
edge node is unwilling to carry any HTLCs for that asset_id.

  1. type: ?? (tap_req_accept)
  2. data:
    • [32*byte:rfq_id]

where:

  • rfq_id is the quote ID that they wish to reject
@ffranr ffranr added enhancement New feature or request rfq labels Feb 1, 2024
@ffranr ffranr self-assigned this Feb 1, 2024
@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

On the tap_rfq_accept.expiry_seconds field:

I don't we're representing expiry optimally. Right now, expiry_seconds is the number of seconds until the quote expires. But if there's any kind of delay in sending/receiving the message, then that expiry will be interpreted inaccurately. Further, the message wouldn't be "self contained", we need to track the receive time somehow.

I think we should have a field called expiry_unix_timestamp with type uint64. It's value would be the unix timestamp after which the quote is no longer valid.

@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

I don't understand the motivation for the tap_rfq_accept.rfq_sig field. Is the idea that this quote accept could be distributed and verified amongst peers? If that's the motivation then I think we should sign over the asset ID/asset group and lot size (asset amount) also. And then changing the expiry becomes important: #780 (comment)

@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

I think tap_rfq_reject should contain an optional error message so that a node can provide feedback.

@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

In choosing the custom message type numbers we might want to consider doing the following:

Custom message types start at 32768. We can specify a taproot-assets specific offset from that starting number by concatenating the alphabetical index positions of the letters "t" (20), "a" (1), and "p" (16), which gives 20116. And then the tap offset is TapMessageTypeBaseOffset = 32768 + 20116.

Using that offset, the quote request message can have type TapMessageTypeBaseOffset + 0, quote accept with TapMessageTypeBaseOffset + 1, and quote reject with TapMessageTypeBaseOffset + 2.

@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

On tap_rfq.suggested_rate_tick:

I think suggested_rate_tick should be renamed to suggested_scaled_exchange_rate.

I think the word "tick" has a particular meaning in finance that confuses what we're trying to explain here. We could use the word pip instead, for example:

`suggested_scaled_exchange_rate` is the internal representation used for asset
conversions. A pip, in this context, is 1/10000th of a currency unit, providing
up to 4 decimal places of precision (0.0001). For example, if the BTC/USD
exchange rate is $61,234.95, we multiply it by 10,000 to determine the
`scaled_exchange_rate`: `$61,234.95 * 10000 = 612,349,500`.  To convert back to
our normal rate, we divide by `10,000` to arrive back at `$61,234.95`.

We might also want to also consider adding a scaling exponent field (uint8) so that we can support variable precision. Otherwise it might be complicated to increase precision in a later release and maintain backwards compatibility.

(Similarly for tap_rfq_accept.accepted_rate_tick.)

@ffranr
Copy link
Contributor Author

ffranr commented Feb 1, 2024

These comments are also on the BLIP PR: lightning/blips#29
I've added them to this issue so that I can keep track of them.

@Roasbeef please let me know what you think when you have time.

@Roasbeef
Copy link
Member

Roasbeef commented Feb 2, 2024

I think we should have a field called expiry_unix_timestamp with type uint64. It's value would be the unix timestamp after which the quote is no longer valid.

I think that makes sense. No strong feelings around renaming or not, but this was the intended semantics. Consider that any price quote returned from the PriceOracle API should also have an expiration date, which is where we'd thread that into the rfq message.

I don't understand the motivation for the tap_rfq_accept.rfq_sig field. Is the idea that this quote accept could be distributed and verified amongst peers?

The intent was to authenticate the data, which can also allow portions of the implementation to be more stateful. They can just keep track of the set of relevant IDs, then verify incoming signatures of the quotes send in the onion payload. This was for an earlier version of the system though, and then I realized we can just move into using the scid trick instead.

I think we can leave it on for now, and just not specify it. But then specify that if present in the TLVs, then it should be verified against the TLV serialization with the sig field stripped out.

I think tap_rfq_reject should contain an optional error message so that a node can provide feedback.

I think an error code w/ some optional metadata is better as we're able handle the error code programmatically vs needing to parse out some custom fields.

In choosing the custom message type numbers we might want to consider doing the following:

What does this achieve? I don't think we need to pin down the exact numbers until later in development, as we'd move closer to finalizing the bLIP. No strong feelings at all on exactly what integer range the messages take on. At present, we have no issues re protocols colliding that integer range.

I think suggested_rate_tick should be renamed to suggested_scaled_exchange_rate.

Seems minor, this is something pretty deep in the protocol. We can get feedback on the API level if needed, though it wouldn't be visible there, as we'd convert w/e rate into the tick format.

We might also want to also consider adding a scaling exponent field (uint8) so that we can support variable precision. Otherwise it might be complicated to increase precision in a later release and maintain backwards compatibility.

Good point. I think an open question is: exactly how much precision is actually needed? For USD quotes, you maybe just need up to 3 or 4 decimal places.

@dstadulis dstadulis added this to the v0.4 milestone Feb 2, 2024
@dstadulis dstadulis moved this from 🆕 New to 💇‍♂️Needs Shaping in Taproot-Assets Project Board Feb 2, 2024
@ffranr ffranr moved this from 💇‍♂️Needs Shaping to 🏗 In progress in Taproot-Assets Project Board Feb 5, 2024
@ffranr ffranr moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Feb 29, 2024
@ffranr ffranr moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Feb 29, 2024
@ffranr ffranr closed this as completed Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfq
Projects
Status: ✅ Done
Development

No branches or pull requests

3 participants