blip-29: update RFQ messages #4
Conversation
ffranr
force-pushed
the
rfq-msg-update
branch
2 times, most recently
from
638e0e9
to
efca73f
Compare
ffranr
changed the title
ffranr
force-pushed
the
rfq-msg-update
branch
from
efca73f
to
fad5262
Compare
ffranr
force-pushed
the
rfq-msg-update
branch
2 times, most recently
from
3bcbefd
to
6355a64
Compare
This commit introduces several changes to the RFQ p2p messages. This includes version fields, group key asset specifiers, in/out assets, max volume, replacing rate tick with fixed-point, and adding a transfer type field. This commit also modifies other RFQ sections such that they are in-line with these new RFQ message fields.
ffranr
force-pushed
the
rfq-msg-update
branch
from
6355a64
to
823a08f
Compare
There was a problem hiding this comment.
Nice work!
Main comment is that we should make it clear that any instances of * or / are done using the fixed point operations. We can make a preliminary section, introduce the FixedPoint abstraction, and then the operations. From then on, we'd then implicitly refrences the fixed point operations with * and /.
| | 11 | in_asset_group_key | 33*byte | | ||
| | 13 | out_asset_id | 32*byte | | ||
| | 15 | out_asset_group_key | 33*byte | | ||
| | 16 | max_transferable_in_asset | BigSize | |
There was a problem hiding this comment.
As it stands, I don't believe we require or use a minimum. I'm not entirely clear on the value of having one, especially since the quote might not be used at all. A minimum volume doesn't seem to provide any protection for the edge node, and it appears to complicate MPP payments unnecessarily.
Could you please clarify the use case you had in mind?
| representation of the conversion rate. | ||
|
|
||
| When serialized into a wire message, the fixed-point number is represented as a | ||
| concatenation of its coefficient and scale fields: `<uint64><uint8>`. |
There was a problem hiding this comment.
I think we should expand this section with the update examples (send + recv) from the tapd docs: https://github.com/lightninglabs/taproot-assets/blob/main/docs/rfq-and-decimal-display.md#sell-order-paying-invoices. In particular the arithmetic operations needed to carry out the flows.
There was a problem hiding this comment.
I think we should also include either just to Go code or abstracted version to ensure that the div+mul operations are clear: https://github.com/lightninglabs/taproot-assets/blob/0d645983d10e0f944bad5e18b0cebeb47d31be3a/rfqmath/fixed_point.go#L75-L99
There was a problem hiding this comment.
I think we should expand this section with the update examples (send + recv) from the
tapddocs: https://github.com/lightninglabs/taproot-assets/blob/main/docs/rfq-and-decimal-display.md#sell-order-paying-invoices. In particular the arithmetic operations needed to carry out the flows.
Do the First Hop TAP HTLC Onion Processing and Last Hop TAP HTLC Onion Processing sections capture the examples that you were thinking of?
| * `amt_to_forward = 1_633_054_326` | ||
| - MUST reject the payment if | ||
| ``` | ||
| amt_msat_to_forward > (amt_asset_incoming / incoming_asset_to_btc) * btc_to_msat_multiplier |
There was a problem hiding this comment.
We should specify that all * and / operations are to be carried out using the FixedPoint abstraction. See comment above, I think we can add it as a prelimnary, then just use * and / as normal from there on.
| `$61,234.95 = 1BTC`). | ||
|
|
||
| The user calculates the invoice amount as follows: | ||
| * `invoice_amt = (asset_amt / asset_to_btc_rate) * btc_to_msat_multiplier` |
There was a problem hiding this comment.
Ah ok I see the example expanded on here.
This commit introduces several changes to the RFQ p2p messages. This includes version fields, group key asset specifiers, in/out assets, max volume, replacing rate tick with fixed-point, and adding a transfer type field.
This commit also modifies other RFQ sections such that they are in-line with these new RFQ message fields.