-
Notifications
You must be signed in to change notification settings - Fork 137
Persist agreed quotes in RFQ service #1863
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
base: main
Are you sure you want to change the base?
Conversation
3c33ca3 to
f0acc02
Compare
Pull Request Test Coverage Report for Build 20035184886Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 19038516198Details
💛 - Coveralls |
98bcb62 to
5a2bd52
Compare
5a2bd52 to
f7ebd34
Compare
f7ebd34 to
703a103
Compare
e74f4ab to
497b820
Compare
2b3ac4f to
035a840
Compare
497b820 to
7beb886
Compare
GeorgeTsagk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits, will do a final pass soon
GeorgeTsagk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, dropped a few more comments
7beb886 to
d223874
Compare
75b90ba to
9023608
Compare
9023608 to
0b0b19d
Compare
ffranr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more trivial points to consider, otherwise this PR is pretty much there IMO.
rfq/order.go
Outdated
| // QuoteID returns the RFQ identifier that originated this policy. | ||
| func (c *AssetPurchasePolicy) QuoteID() (rfqmsg.ID, bool) { | ||
| return c.AcceptedQuoteId, true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is called anywhere so it can be removed here and in the Policy interface. I might be wrong. Why does this have a bool return value? We could use fn.Option perhaps? But I think this method can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true! Residue of the old implementation with active field. Removed.
GeorgeTsagk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some last optional nits provided below
0b0b19d to
521bafc
Compare
521bafc to
0fb9015
Compare
Adds a new rfq_policies table and store allowing persistence of sale and purchase policies.
refactors the current quote in-memory storage away from the Manager into the OrderHandler to encapsulate all quote storage operations in the same component.
0fb9015 to
15a5f3c
Compare
RFQ buy/sell accepts are now written to the database (
rfq_policiestable) whenever a policy is agreed, giving us an audit trail and keeping quotes across restarts.The
OrderHandlerreloads these rows at startup to rebuild policies and the manager repopulates its in-memory caches from the same data.Added a check in
itestthat restarts Bob’stapdmid RFQ flow and verifies the quote remains available to Carol.Fixes #855