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.

Sign up for GitHub

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

Jump to bottom

A minimal version of the RFQ system #767

Merged
merged 8 commits into from
Feb 29, 2024
Merged

A minimal version of the RFQ system #767

merged 8 commits into from
Feb 29, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Jan 16, 2024 •
edited
Loading

Closes #766

This PR adds a minimal version of the RFQ system.

@ffranr ffranr force-pushed the rfq-send-recv branch 3 times, most recently from 7d36f26 to 88ebebd Compare January 17, 2024 17:49
@dstadulis
Copy link
Collaborator

dstadulis commented Jan 17, 2024 •
edited
Loading

TODO:

  • Architecture feedback on the RFQ handler

@dstadulis dstadulis requested a review from Roasbeef January 17, 2024 19:37
@ffranr ffranr force-pushed the rfq-send-recv branch 3 times, most recently from da10dba to c621cfb Compare January 22, 2024 15:23
@ffranr ffranr requested a review from guggero January 22, 2024 16:45
guggero
guggero reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice progress on this!
Doing things fully concurrently is pretty hard. I think the way the three main components are currently set up is probably quite hard to understand and error prone. So I think we should either merge the three components together (e.g. a single main event loop goroutine if possible) or make them more independent (both sub components have a start/stop method and manage their own goroutine, so be fully independent components).

chain_bridge.go Outdated Show resolved Hide resolved
rfqmessages/messages.go Outdated Show resolved Hide resolved
rfqmessages/req_accept.go Outdated Show resolved Hide resolved
rfqmessages/messages.go Outdated Show resolved Hide resolved
rfqservice/log.go Outdated Show resolved Hide resolved
rfqservice/quote_negotiator.go Outdated Show resolved Hide resolved
rfqservice/rfq.go Outdated Show resolved Hide resolved
rfqservice/rfq.go Outdated Show resolved Hide resolved
rfqservice/rfq.go Outdated Show resolved Hide resolved
rfqservice/stream.go Outdated Show resolved Hide resolved
@dstadulis dstadulis added this to the v0.4 milestone Jan 24, 2024
@dstadulis
Copy link
Collaborator

ffranr is updating based on guggero's review

@ffranr ffranr added the rfq label Jan 29, 2024
@ffranr ffranr changed the base branch from main to approved_rfq January 30, 2024 13:24
@ffranr ffranr force-pushed the rfq-send-recv branch 3 times, most recently from 8ca9028 to b5b6792 Compare February 6, 2024 19:46
@ffranr ffranr changed the title WIP: RFQ stream handler WIP: RFQ Feb 6, 2024
Roasbeef
Roasbeef reviewed Feb 9, 2024
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! Completed an initial drive by, dropped some relevant comments and fresh framing to help us synchronize our mental models.

chain_bridge.go Outdated Show resolved Hide resolved
chain_bridge.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
rfqmsg/accept.go Outdated Show resolved Hide resolved
rfqmsg/accept.go Outdated Show resolved Hide resolved
rfqservice/order.go Outdated Show resolved Hide resolved
rfqservice/order.go Outdated Show resolved Hide resolved
rfqservice/stream.go Outdated Show resolved Hide resolved
rfqservice/stream.go Outdated Show resolved Hide resolved
tapdb/sqlc/migrations/000015_rfq.up.sql Outdated Show resolved Hide resolved
@ffranr
Copy link
Contributor Author

ffranr commented Feb 14, 2024 •
edited
Loading

The latest commits include an itest which:

  • negotiates a quote via RFQ system between two peers
  • buy side peer uses an accepted quote to send HTLC over quote specified (scid) channel
  • sell side peer validates HTLC based on accepted quote in RFQ system

@ffranr ffranr force-pushed the rfq-send-recv branch 5 times, most recently from ddf3fbe to aed3714 Compare February 19, 2024 15:56
@dstadulis dstadulis requested a review from jharveyb February 19, 2024 19:07
guggero
guggero reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful code and very nice commit structure.
Truly amazing work on this, congrats 🎉 Reads very well!

rfq/manager.go Outdated Show resolved Hide resolved
rfq/manager.go Outdated Show resolved Hide resolved
rfq/manager.go Show resolved Hide resolved
rfq/manager.go Show resolved Hide resolved
rfq/manager.go Show resolved Hide resolved
taprpc/rfqrpc/rfq.proto Outdated Show resolved Hide resolved
taprpc/rfqrpc/rfq.proto Outdated Show resolved Hide resolved
taprpc/rfqrpc/rfq.yaml Outdated Show resolved Hide resolved
itest/rfq_test.go Outdated Show resolved Hide resolved
itest/rfq_test.go Outdated Show resolved Hide resolved
@ffranr ffranr requested a review from guggero February 23, 2024 13:13
@ffranr
Copy link
Contributor Author

ffranr commented Feb 23, 2024

Thank you Oli for your detailed review!

@jharveyb FYI my goal with this PR is to get it merged so I can get back to sprints with manageable sized issues and smaller PRs. So I'd prefer to only make changes towards a merge and no extra features, if you see what I mean. The RFQ system won't be used immediately anyway.

@ffranr ffranr force-pushed the rfq-send-recv branch 6 times, most recently from aaa9970 to de667f3 Compare February 27, 2024 12:34
jharveyb
jharveyb reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Pretty neat to see the bLIP coming to life 💯

Left a few Qs and notes, should be minor changes.

rfqmsg/messages.go Outdated Show resolved Hide resolved
rfqmsg/request.go Show resolved Hide resolved
rfqmsg/request.go Show resolved Hide resolved
rfqmsg/accept.go Outdated Show resolved Hide resolved
rfqmsg/accept_test.go Outdated Show resolved Hide resolved
rfq/negotiator.go Show resolved Hide resolved
rfq/manager.go Show resolved Hide resolved
rfq/manager.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
guggero
guggero approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending nits and existing comments from @jharveyb.

rfqmsg/accept.go Outdated Show resolved Hide resolved
rfqmsg/request_test.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
@ffranr ffranr requested a review from jharveyb February 28, 2024 23:21
jharveyb
jharveyb approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid changes given the feedback, and all points not changed bot tagged with TODOs for follow-up PRs in this series / line of work 💯

rfq/order.go Outdated Show resolved Hide resolved
@ffranr
asset: improve code formatting
3bd1090
@ffranr
itest: fix comment
1c67efd
@ffranr
rpcserver: fix log message
d6c994f
@ffranr
rfqmsg: add new RFQ message types package
596805d 
This commit adds a new package called rfqmsg which contains RFQ message
types. Message types are: request, accept, and reject.
@ffranr
rfq: add new RFQ service package
47d74e5 
This commit adds a new package called rfq which contains the RFQ
service. Service sub-systems include:
1. Central service manager.
2. Quote price negotiator (and price oracle).
3. Peer message manager (stream handler).
4. HTLC order manager.
@ffranr
multi: start the RFQ service on startup
ff71f2b 
This commit plugs the new RFQ service into the greater tapd service.
@ffranr
rpc: add RPC endpoints for the RFQ system
a73aac8 
This commit adds the RFQ RPC server. It also adds four RPC endpoints:
1. Upsert buy order.
2. Upsert sell offer.
3. List accepted quotes.
4. Subscribe to RFQ events.
@ffranr
itest: add RFQ system HTLC interception test
5b6872b 
This commit adds an itest which tests that the RFQ system can be used to
reach an agreement on a quote between two peers and then validate the
corresponding lightning payment HTLC.
@ffranr ffranr added this pull request to the merge queue Feb 29, 2024
Merged via the queue into main with commit 10f0e3a Feb 29, 2024
14 checks passed
@guggero guggero deleted the rfq-send-recv branch March 1, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@Roasbeef Roasbeef Roasbeef left review comments

@jharveyb jharveyb jharveyb approved these changes

Assignees

@ffranr ffranr

Labels
payment-channel rfq
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

[feature]: create StreamHandler RFQ service (send/recv lnd custom msg)
5 participants
@ffranr @dstadulis @Roasbeef @guggero @jharveyb