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

RFQ refactor towards adding asset sell request support #845

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Mar 19, 2024

Towards fixing #818

This PR contains commits which refactor the RFQ service towards solving #818 .

The changes in this PR shouldn't change the functionality of the RFQ service. They should rename methods, functions, types, and variables so that the changes necessary for #818 are easier to make in a different PR.

@ffranr ffranr force-pushed the rfq-refactor-for-sell-support branch from 1ed8c7d to a0e40e8 Compare March 19, 2024 18:53
@guggero guggero self-requested a review March 19, 2024 18:56
@ffranr ffranr self-assigned this Mar 19, 2024
@ffranr ffranr force-pushed the rfq-refactor-for-sell-support branch 2 times, most recently from 45edb8c to 681e4b0 Compare March 19, 2024 19:12
@ffranr ffranr requested a review from GeorgeTsagk March 19, 2024 19:23
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.

Changes look good with the exception of the last commit (see inline comment).

rpcserver.go Outdated Show resolved Hide resolved
taprpc/rfqrpc/rfq.proto Outdated Show resolved Hide resolved
rfqmsg/messages.go Show resolved Hide resolved
chain_bridge.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the rfq-refactor-for-sell-support branch 2 times, most recently from 0896416 to 8323b0b Compare March 20, 2024 15:59
@ffranr ffranr requested a review from guggero March 20, 2024 16:09
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.

Needs a rebase, otherwise LGTM 🎉

Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM ⚡
few nits

rfqmsg/messages.go Outdated Show resolved Hide resolved
// Given valid RFQ message id, we then define a RFQ short chain id
// (SCID) by taking the last 8 bytes of the RFQ message id and
// interpreting them as a 64-bit integer.
scidBytes := id[24:]
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
scidBytes := id[24:]
scidBytes := id[len(id)-8]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think your suggesting works. We want to select the last 8 elements from the id array, not the single element at len(id)-8.

ffranr added 8 commits March 21, 2024 09:59
We need to generalise our reject message constructor so that we can
generate reject messages for the new sell request message type in
addition to the existing buy request message type.
This commit renames the RFQ event type `IncomingAcceptQuoteEvent` to
`PeerAcceptedBuyQuoteEvent` such that it is "buy" specific. We will add
a new "sell" specific event type in a future commit.
In this commit, we rename methods, RPC endpoints, and variables to
clarify that the subject quotes are those that were requested by our
node and were accepted by our peers.

They are not quotes that our node has accepted. Quotes that our node has
accepted are stored as sale/purchase policies in the RFQ order handler.
In this commit we rename a test and update its doc to clarify that the
payment made as part of the test is initiated via an asset buy RFQ
request. This change is made in preparation for adding a future itest
where a payment is made following an asset sell RFQ request.
In this commit we ensure that the mock price oracle returns an ask price
when queried even if a suggested bid price is not provided.
This commit makes three changes:
1. Drop the word "remit" in favour of the more well known word "policy".
2. Make the policy struct asset sale specific to make room for an asset
purchase policy which will be added in a future commit.
3. Add new `Policy` interface so that the code remains simple going
forward.
The short channel ID (SCID) is derived from the message ID. We add a
method to the message ID type in an effort to remove duplication of the
SCID derivation code.
@ffranr ffranr force-pushed the rfq-refactor-for-sell-support branch 2 times, most recently from bf2b678 to 13dddf3 Compare March 21, 2024 10:46
@ffranr
Copy link
Contributor Author

ffranr commented Mar 21, 2024

Last commit (lndclient version bump) wasn't necessary after rebase.

@ffranr ffranr added this pull request to the merge queue Mar 21, 2024
Merged via the queue into main with commit 1656733 Mar 21, 2024
14 checks passed
@guggero guggero deleted the rfq-refactor-for-sell-support branch March 21, 2024 11:44
@dstadulis dstadulis mentioned this pull request Mar 26, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants