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

More RFQ improvments #1197

Merged
merged 15 commits into from
Nov 19, 2024
Merged

More RFQ improvments #1197

merged 15 commits into from
Nov 19, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Nov 15, 2024

Improve docs for: SellOrder, SellRequest, BuyOrder, BuyRequest.
Populate transfer type field in request.
Make use of AssetRate type.

@ffranr ffranr requested a review from guggero November 15, 2024 15:19
@ffranr ffranr self-assigned this Nov 15, 2024
@ffranr
Copy link
Contributor Author

ffranr commented Nov 15, 2024

Things left to think about:

  • request fields: min in-asset and min out-asset.
  • Use transfer type to determine buy/sell. Remove need for all zeros asset ID to signify BTC if we can just use transfer type? BLIP?

@dstadulis dstadulis added the documentation Improvements or additions to documentation label Nov 15, 2024
@coveralls
Copy link

coveralls commented Nov 15, 2024

Pull Request Test Coverage Report for Build 11913943259

Details

  • 24 of 163 (14.72%) changed or added relevant lines in 13 files are covered.
  • 22 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.04%) to 41.059%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannel/aux_invoice_manager.go 1 2 50.0%
rfqmsg/buy_request.go 0 2 0.0%
rfqmsg/sell_request.go 0 2 0.0%
tapchannel/aux_traffic_shaper.go 0 2 0.0%
rfq/order.go 0 4 0.0%
rfqmsg/messages.go 19 23 82.61%
rfq/manager.go 0 5 0.0%
rfqmsg/buy_accept.go 2 7 28.57%
rfqmsg/sell_accept.go 0 8 0.0%
rfqmsg/accept.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
rfqmsg/sell_accept.go 1 0.0%
rpcserver.go 1 0.0%
asset/asset.go 2 80.96%
tapgarden/caretaker.go 8 68.5%
universe/interface.go 10 51.12%
Totals Coverage Status
Change from base Build 11896639646: 0.04%
Covered Lines: 25199
Relevant Lines: 61372

💛 - Coveralls

@ffranr ffranr requested a review from GeorgeTsagk November 18, 2024 12:35
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.

Very nice cleanup

Could you rebase so that we can see the LitD itests running against this too?

rfqmsg/messages.go Show resolved Hide resolved
Refactor SellOrder by replacing the fields "asset ID" and "asset
group key" with a single "asset specifier" field for simplicity
and consistency.
Currently, the `SellOrder.Peer` field must be specified, but in the
future, the negotiator should be able to select the optimal peer
automatically. This commit updates the interface to support this future
functionality by making `SellOrder.Peer` an Option type. Note that this
field was already a pointer.
This commit removes the `peer` argument from the methods
queryBidFromPriceOracle and queryAskFromPriceOracle
Populate the expiry timestamp field in SellOrder and use type
 `time.Time`.
@ffranr ffranr force-pushed the rfq-improvments-20241115 branch from df78d25 to 0ef0264 Compare November 19, 2024 11:33
@ffranr ffranr requested a review from GeorgeTsagk November 19, 2024 12: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.

Very nice, thanks for the additional cleanup!
One request and one nit, so we're quite close 🙏

rfqmsg/messages.go Show resolved Hide resolved
rfqmsg/buy_accept.go Outdated Show resolved Hide resolved
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!

Refactor BuyAccept by replacing multiple fields with a single
asset rate field using the new AssetRate type.
When handling a quote request accept message, the peer-provided
price must be validated against our price oracle. This commit
passes the peer-provided price as a hint to the oracle, offering
additional context to improve decision-making.
Improve doc by relating these types to the wider lightning payment flow
context.
Populate the expiry timestamp field in BuyOder and use type
`time.Time`.
Improve doc by relating this type to the wider lightning payment flow
context.
Refactor SellAccept by replacing multiple fields with a single
asset rate field using the new AssetRate type.
Changed the `expiry` argument in `expiryWithinBounds` to be of type
`time.Time` for improved type safety and clarity.
Set the converted `time.Time` to UTC when transforming from Unix time.
This change promotes consistency and simplifies debugging.
Ensure the request message version check validates only a specific
version number, rather than accepting any version greater than or
equal to the latest message number.
@ffranr ffranr force-pushed the rfq-improvments-20241115 branch from 0ef0264 to abf47dc Compare November 19, 2024 12:52
@ffranr ffranr enabled auto-merge November 19, 2024 12:53
@ffranr ffranr requested a review from guggero November 19, 2024 12:53
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.

Sorry, found one last thing. Other than that LGTM 🎉

rfqmsg/messages.go Outdated Show resolved Hide resolved
rfqmsg/request.go Show resolved Hide resolved
@ffranr ffranr added this pull request to the merge queue Nov 19, 2024
@ffranr ffranr removed this pull request from the merge queue due to a manual request Nov 19, 2024
Update the buy/sell classification logic for incoming requests to rely
on the new transfer type field.

When the requesting peer attempts to pay an invoice using a Tap asset,
they are "selling" the Tap asset to the edge node. Conversely, when the
requesting peer attempts to receive a Tap asset as payment to settle an
invoice, they are "buying" the Tap asset from the edge node.
@ffranr ffranr force-pushed the rfq-improvments-20241115 branch from abf47dc to a1d989d Compare November 19, 2024 13:20
@ffranr ffranr enabled auto-merge November 19, 2024 13:21
@ffranr ffranr added this pull request to the merge queue Nov 19, 2024
Merged via the queue into main with commit a304937 Nov 19, 2024
18 checks passed
@guggero guggero deleted the rfq-improvments-20241115 branch November 19, 2024 14:50
gijswijs pushed a commit that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants