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 session lookup during quote accept message parsing #1128

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Sep 18, 2024

This PR is not a breaking change. It's related to #871

Future RFQ quote accept messages will not contain enough information for the RFQ service to classify them as buy or sell. Therefore, before these messages can be fully interpreted, the corresponding quote request message must be retrieved and inspected.

This commit modifies the parsing of incoming quote accept messages so they can be accurately classified as buy or sell by looking up the associated quote request message.

As a beneficial side effect, parsed quote accept message fields are now fully populated within the parsing routine.

@ffranr ffranr self-assigned this Sep 18, 2024
@dstadulis dstadulis added this to the v0.4.2 milestone Sep 18, 2024
@coveralls
Copy link

coveralls commented Sep 18, 2024

Pull Request Test Coverage Report for Build 10941878808

Details

  • 0 of 56 (0.0%) changed or added relevant lines in 8 files are covered.
  • 15 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.03%) to 40.422%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rfqmsg/buy_accept.go 0 2 0.0%
rfqmsg/sell_accept.go 0 2 0.0%
rfqmsg/messages.go 0 3 0.0%
rfqmsg/buy_request.go 0 6 0.0%
rfqmsg/reject.go 0 6 0.0%
rfqmsg/sell_request.go 0 6 0.0%
rfq/stream.go 0 13 0.0%
rfqmsg/accept.go 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
rfqmsg/accept.go 1 37.35%
internal/test/helpers.go 2 86.85%
tappsbt/create.go 2 53.22%
commitment/tap.go 2 84.43%
tapgarden/caretaker.go 4 68.5%
asset/asset.go 4 81.61%
Totals Coverage Status
Change from base Build 10941480416: 0.03%
Covered Lines: 24223
Relevant Lines: 59925

💛 - Coveralls

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.

Nice refactor! Will come in handy with the following RFQ message changes, I'm sure.

rfq/stream.go Outdated Show resolved Hide resolved
rfqmsg/accept.go Outdated Show resolved Hide resolved
rfqmsg/messages.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the rfqmsg-session-lookup-parsing branch from b92c0cb to b91ebfd Compare September 19, 2024 09:12
This modification simplifies incoming message handling in future
commits.
Future RFQ quote accept messages will not contain enough information
for the RFQ service to classify them as buy or sell. Therefore, before
these messages can be fully interpreted, the corresponding quote request
message must be retrieved and inspected.

This commit modifies the parsing of incoming quote accept messages so
they can be accurately classified as buy or sell by looking up the
associated quote request message.

As a beneficial side effect, parsed quote accept message fields are now
fully populated within the parsing routine.
@ffranr ffranr force-pushed the rfqmsg-session-lookup-parsing branch from b91ebfd to 2eb9eea Compare September 19, 2024 13:21
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.

@ffranr ffranr added this pull request to the merge queue Sep 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 20, 2024
@guggero guggero merged commit 0d64598 into main Sep 20, 2024
18 checks passed
@guggero guggero deleted the rfqmsg-session-lookup-parsing branch September 20, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants