Add asset sell support to the RFQ service #835

ffranr · 2024-03-14T15:14:44Z

Closes #818

This PR builds on top of the refactoring work from #814 .

It adds asset sell request and sell request accept support to the RFQ service.

dstadulis · 2024-03-14T15:41:33Z

There's local code
need to rebase code
getting the itest

ffranr · 2024-03-18T20:22:19Z

I'm not sure how to setup the itest for the case where we sell a tap asset as the payment to satisfy the first hop in a lightning payment route.

Alice wants to pay Carol. Carol generates the invoice.

Alice wants to sell a tap asset to Bob, and Alice would like Bob to send the bid payment in that sale to Carol. In so doing Bob would satisfy the invoice that Carol generated for Alice.

I can currently:

Reach a sale quote agreement between Alice and Bob.
Generate an invoice with Carol.
Generate a custom route from Alice, to Bob, and then to Carol.

Bob can use his RFQ service to listen for a HTLC from Alice that satisfies the agreed Alice sale quote. And Alice can create a custom route with an input amount of 0 to Bob, but then stipulate that Bob must pay Carol the balance agreed upon in Alice-Bob's sale quote.

Is that the way forward? @guggero ?

guggero · 2024-03-18T20:44:18Z

There are important parts missing on the lnd side for this to work fully. So we need to find a compromise so we can test something. I think what we discussed offline at some point was that for now Alice would embed custom data to Bob (the same way you can embed dest_custom_records for the final receiver, we'd want to include a custom TLV record for Bob, I think we found that the SendToRoute might allow that or might need some additions to allow for it).
The custom data at this moment could just be the RFQ ID. Then Bob's tapd could intercept the HTLC, decode the custom record data and check that an offer with that RFQ ID exists, then check that the outgoing amount matches that offer.

ffranr · 2024-03-19T12:40:17Z

There are important parts missing on the lnd side for this to work fully. So we need to find a compromise so we can test something. I think what we discussed offline at some point was that for now Alice would embed custom data to Bob (the same way you can embed dest_custom_records for the final receiver, we'd want to include a custom TLV record for Bob, I think we found that the SendToRoute might allow that or might need some additions to allow for it). The custom data at this moment could just be the RFQ ID. Then Bob's tapd could intercept the HTLC, decode the custom record data and check that an offer with that RFQ ID exists, then check that the outgoing amount matches that offer.

@guggero In the asset buy payment initiation case the HTLC intercepting node uses the SCID to identify the relevant HTLC. Both peers are aware of the SCID as it is present in the accepted quote (SCID is derived from message ID). Why do we need custom records? Can't we just forward a HTLC over the accepted quote SCID?

guggero · 2024-03-19T16:54:20Z

@guggero In the asset buy payment initiation case the HTLC intercepting node uses the SCID to identify the relevant HTLC. Both peers are aware of the SCID as it is present in the accepted quote (SCID is derived from message ID). Why do we need custom records? Can't we just forward a HTLC over the accepted quote SCID?

Oh, you're right, we have the incoming_channel_id, I wasn't aware of that. I thought we'd only have the outgoing one, which in this case would be a "real" channel in the network. But then yes, we can just use the quote SCID to match things, no need for custom records.

ffranr · 2024-03-19T17:50:41Z

@guggero In the asset buy payment initiation case the HTLC intercepting node uses the SCID to identify the relevant HTLC. Both peers are aware of the SCID as it is present in the accepted quote (SCID is derived from message ID). Why do we need custom records? Can't we just forward a HTLC over the accepted quote SCID?

Oh, you're right, we have the incoming_channel_id, I wasn't aware of that. I thought we'd only have the outgoing one, which in this case would be a "real" channel in the network. But then yes, we can just use the quote SCID to match things, no need for custom records.

I've managed to get the custom records method to work with a custom route. But I didn't manage to forward a HTLC when I set ChanId on the custom route or set the incoming channel on a hop hint. Not sure what's going on there, perhaps LND is rejecting the channel ID given that an actual channel exists.

ffranr · 2024-03-19T17:55:15Z

I wander if the itest error is because of some side effect from the other RFQ itest. Passes locally for me.

The first 10 commits can be split out into a separate PR. They are effectively refactor commits.

dstadulis · 2024-03-19T18:38:03Z

itests are failing when running together

dstadulis · 2024-03-19T18:38:29Z

@ffranr will split out PRs to get merge faster

guggero

Looks pretty good, very nice work 🎉
Just a few minor comments

rfqmsg/sell_request.go

rfq/negotiator.go

rfqmsg/sell_accept.go

rfq/negotiator.go

taprpc/rfqrpc/rfq.proto

rpcserver.go

itest/rfq_test.go

GeorgeTsagk

Looks good, a few Qs
very nice commit structure

rfq/manager.go

rfq/negotiator.go

itest/rfq_test.go

This commit adds an RPC endpoint for uploading an asset sell order to the RFQ service manager. This asset sell order will be converted into a sell request (or multiple) by the RFQ service negotiator.

Up until this commit in the PR, we've constructed sell request messages and have sent them to our peer (outgoing). In this commit we start modifying the RFQ service such that it can also receive those sell request messages from peers.

We will use this new type in a subsequent commit where we respond to an incoming sell request message.

In this commit we enhance the RFQ service such that the manager directs an incoming (from a peer) asset sell quote request message to the RFQ negotiator sub-service. The negotiator then populates the outgoing messages channel with a sell request accept message. This message will eventually be forwarded to the peer by the streamer RFQ sub-service.

This commit adds a new policy type to the order handler RFQ service. The new policy type is specific to asset purchases (where the counterparty has requested to sell an asset to our node). With this new type we can check that a HTLC is valid in the context of an asset purchase specifically. In this commit we ensure that before an asset sell quote request accept message is sent to our peer, the RFQ order handler registers a new asset purchase policy.

In this commit, we enable parsing of sell quote request accept messages from wire messages. We also store incoming (sent from our peer) asset sell quote request messages in the RFQ manager.

In this commit we add a new RFQ event type `PeerAcceptedSellQuoteEvent`. This event is published to RFQ event subscribers when the RFQ service receives an asset sell quote request accept message from a peer.

In this commit we extend RPC endpoint `QueryPeerAcceptedQuotes` such that it returns peer accepted sell quotes in addition to the existing peer accepted buy quotes. This change also includes expanding the RFQ manager `QueryPeerAcceptedQuotes` method return values.

The goal of this commit is to simplify troubleshooting LND node management.

The RFQ test scenario generates new LND nodes. In this commit we modify the test scenario cleanup routine to ensure that those nodes are killed at the end of each test.

ffranr · 2024-03-26T19:17:31Z

Some of the review comments also apply to existing code that the PR doesn't touch. I've made a note of them and I'll create another PR address those code changes.

guggero

Great work! LGTM pending unit tests for encode/decode of messages.

GeorgeTsagk

Looks good!

are the encode/decode unit tests landing in a diff PR?
otherwise lgtm

ffranr · 2024-03-28T09:57:41Z

@GeorgeTsagk thanks for taking a second look.

I've added these unit tests for encode/decode in this PR:
https://github.com/lightninglabs/taproot-assets/pull/835/files#diff-26f6b4e4b5386817099becddf822524f93feec1169c2f0901380dabb7c25d740

https://github.com/lightninglabs/taproot-assets/pull/835/files#diff-4ee5dc5905f2ad2d13635df68e16d1881e3ceedde4ab1f91529d7b95f0c7396a

ffranr changed the title ~~RFQ add sell support~~ RFQ add asset sell support Mar 14, 2024

ffranr force-pushed the rfq-add-sell-support branch 2 times, most recently from c37a1fd to cbc287a Compare March 14, 2024 15:20

ffranr changed the title ~~RFQ add asset sell support~~ Add asset sell support to the RFQ service Mar 14, 2024

dstadulis added the rfq label Mar 14, 2024

Base automatically changed from rfq-sell-asset to main March 15, 2024 11:03

ffranr force-pushed the rfq-add-sell-support branch 5 times, most recently from 08b298f to 35e9ce6 Compare March 18, 2024 19:34

ffranr force-pushed the rfq-add-sell-support branch 2 times, most recently from 746b5af to d2bc336 Compare March 19, 2024 16:11

ffranr force-pushed the rfq-add-sell-support branch from d2bc336 to a9a93fe Compare March 19, 2024 17:42

ffranr force-pushed the rfq-add-sell-support branch from a9a93fe to 4c382c4 Compare March 19, 2024 18:17

ffranr marked this pull request as ready for review March 19, 2024 18:20

dstadulis requested review from guggero and GeorgeTsagk March 19, 2024 18:39

ffranr self-assigned this Mar 19, 2024

ffranr changed the base branch from main to rfq-refactor-for-sell-support March 19, 2024 18:50

ffranr marked this pull request as draft March 19, 2024 18:51

ffranr requested review from guggero and GeorgeTsagk March 22, 2024 12:48

guggero reviewed Mar 26, 2024

View reviewed changes

GeorgeTsagk reviewed Mar 26, 2024

View reviewed changes

rfq/manager.go Show resolved Hide resolved

rfq/negotiator.go Show resolved Hide resolved

rfq/negotiator.go Show resolved Hide resolved

itest/rfq_test.go Show resolved Hide resolved

itest/rfq_test.go Outdated Show resolved Hide resolved

ffranr force-pushed the rfq-add-sell-support branch 2 times, most recently from 560845d to 611353a Compare March 26, 2024 18:17

ffranr added 13 commits March 26, 2024 19:15

rfqmsg: add asset sell request message type

52348bc

rfq: service accepts a sell order and generates outbound sell requests

ecc38ba

rpc: add endpoint to upload an asset sell order

7876cbc

This commit adds an RPC endpoint for uploading an asset sell order to the RFQ service manager. This asset sell order will be converted into a sell request (or multiple) by the RFQ service negotiator.

rfqmsg: parse incoming sell request type messages

9c2c0ce

Up until this commit in the PR, we've constructed sell request messages and have sent them to our peer (outgoing). In this commit we start modifying the RFQ service such that it can also receive those sell request messages from peers.

rfqmsg: add asset sell request accept message type

40d7a0f

We will use this new type in a subsequent commit where we respond to an incoming sell request message.

rfq+rfqmsg: parse and store incoming sell quote request accept msg

3cd0ad1

In this commit, we enable parsing of sell quote request accept messages from wire messages. We also store incoming (sent from our peer) asset sell quote request messages in the RFQ manager.

rfq+rpc: publish an event when we receive a SellAccept msg from a peer

ffba8fe

In this commit we add a new RFQ event type `PeerAcceptedSellQuoteEvent`. This event is published to RFQ event subscribers when the RFQ service receives an asset sell quote request accept message from a peer.

itest: randomize new LND node names when constructing RFQ test scenario

7de93ea

The goal of this commit is to simplify troubleshooting LND node management.

itest: kill LND nodes during RFQ test scenario cleanup

e34730c

The RFQ test scenario generates new LND nodes. In this commit we modify the test scenario cleanup routine to ensure that those nodes are killed at the end of each test.

itest: test that RFQ can agree on asset sell quote and verify HTLC

ba506bc

ffranr force-pushed the rfq-add-sell-support branch from 611353a to ba506bc Compare March 26, 2024 19:16

guggero approved these changes Mar 26, 2024

View reviewed changes

ffranr requested a review from GeorgeTsagk March 26, 2024 19:44

GeorgeTsagk approved these changes Mar 28, 2024

View reviewed changes

ffranr added this pull request to the merge queue Mar 28, 2024

Merged via the queue into main with commit e5a4a16 Mar 28, 2024
14 checks passed

ffranr mentioned this pull request Mar 28, 2024

RFQ cleanup #860

Merged

guggero deleted the rfq-add-sell-support branch March 28, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asset sell support to the RFQ service #835

Add asset sell support to the RFQ service #835

ffranr commented Mar 14, 2024

dstadulis commented Mar 14, 2024

ffranr commented Mar 18, 2024

guggero commented Mar 18, 2024

ffranr commented Mar 19, 2024 •

edited

Loading

guggero commented Mar 19, 2024

ffranr commented Mar 19, 2024

ffranr commented Mar 19, 2024 •

edited

Loading

dstadulis commented Mar 19, 2024

dstadulis commented Mar 19, 2024

guggero left a comment

GeorgeTsagk left a comment

ffranr commented Mar 26, 2024

guggero left a comment

GeorgeTsagk left a comment

ffranr commented Mar 28, 2024 •

edited

Loading

Add asset sell support to the RFQ service #835

Add asset sell support to the RFQ service #835

Conversation

ffranr commented Mar 14, 2024

dstadulis commented Mar 14, 2024

ffranr commented Mar 18, 2024

guggero commented Mar 18, 2024

ffranr commented Mar 19, 2024 • edited Loading

guggero commented Mar 19, 2024

ffranr commented Mar 19, 2024

ffranr commented Mar 19, 2024 • edited Loading

dstadulis commented Mar 19, 2024

dstadulis commented Mar 19, 2024

guggero left a comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

ffranr commented Mar 26, 2024

guggero left a comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

ffranr commented Mar 28, 2024 • edited Loading

ffranr commented Mar 19, 2024 •

edited

Loading

ffranr commented Mar 19, 2024 •

edited

Loading

ffranr commented Mar 28, 2024 •

edited

Loading