-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add itest oracle harness #1395
Add itest oracle harness #1395
Conversation
Pull Request Test Coverage Report for Build 13795013078Details
💛 - Coveralls |
a76775a
to
269ad4b
Compare
19876aa
to
52c7c50
Compare
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.
Linter is unhappy, otherwise LGTM 🎉
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.
Could we specify different oracle servers for alice, bob, and carol from the get go perhaps?
itest/oracle_harness.go
Outdated
bidPrices map[string]rfqmath.BigIntFixedPoint | ||
askPrices map[string]rfqmath.BigIntFixedPoint |
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 think you should add a comment to explain what string
is here. in other words, I think we should have field level doc comments here (even though it's test code).
52c7c50
to
3351424
Compare
Thanks for the feedback @ffranr , addressed your comments |
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 think you can use asset.Specifier
as a map key. I think all fields are comparable.
269ad4b
to
7c660bd
Compare
3351424
to
80294e4
Compare
We should be careful when comparing fields that have a |
7c660bd
to
6cf7190
Compare
The public oracle server RPC endpoints for querying prices would only accept a specifier that sets only the asset ID. We may now accept group keys as well, so we consider any specifier to be valid. This is needed as we're later going to create a oracle rpc server harness to test out this code path.
In order for the oracle server harness to have a native way of providing us with logs in the itests we need to set up an itest logger, which is also initiated during test harness setup.
Adds a simple oracle server harness, this is a copy of the oracle harness implementation in LitD. We need this to have greatest code coverage in our tapd itests and also to have moving prices during the itest execution, allowing the creation of more complex scenarios.
Not all itests need to use an oracle server harness, so we introduce a functional option that may be defined when setting up a harness. This will reduce the overall diff as previous callers of the harness setup functions won't need to change their arguments.
In order to test the new harness and the previous changes, we introduce the oracle server harness in one of our rfq related tests. The 2 peers in this test will now query a real oracle price server instead of defaulting to their mock.
80294e4
to
4048004
Compare
Description
This PR adds the oracle harness that is used in the LitD itests to our tapd repo. This will allow us to run an actual dummy grpc oracle server that the tapd instances of our itests can use. This will offer a slightly bigger code coverage in itests plus enable more complex scenarios with moving prices.
This branch is based on
rfq-negotiation-groupkey
, which is also set as the base of this GH pull request in order to not see duplicate commits.SubscribeRfqEventNtfns
RPC endpoint will crash if it tries to marshal the rfq message sent by our peer if they consider our price unacceptable.