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

Add itest oracle harness #1395

Merged
merged 6 commits into from
Mar 11, 2025
Merged

Add itest oracle harness #1395

merged 6 commits into from
Mar 11, 2025

Conversation

GeorgeTsagk
Copy link
Member

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.

  • Fix bug: The SubscribeRfqEventNtfns RPC endpoint will crash if it tries to marshal the rfq message sent by our peer if they consider our price unacceptable.

@GeorgeTsagk GeorgeTsagk self-assigned this Feb 17, 2025
@GeorgeTsagk GeorgeTsagk added this to the v0.6 milestone Feb 17, 2025
@coveralls
Copy link

coveralls commented Feb 17, 2025

Pull Request Test Coverage Report for Build 13795013078

Details

  • 170 of 260 (65.38%) changed or added relevant lines in 5 files are covered.
  • 42 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.2%) to 54.772%

Changes Missing Coverage Covered Lines Changed/Added Lines %
itest/test_harness.go 9 11 81.82%
itest/tapd_harness.go 13 17 76.47%
itest/oracle_harness.go 118 202 58.42%
Files with Coverage Reduction New Missed Lines %
commitment/tap.go 1 85.0%
rpcserver.go 2 64.09%
tapgarden/custodian.go 2 75.5%
tapgarden/caretaker.go 4 77.73%
asset/asset.go 5 80.75%
asset/mock.go 6 71.43%
itest/multisig.go 6 97.43%
address/mock.go 16 86.93%
Totals Coverage Status
Change from base Build 13794454068: 0.2%
Covered Lines: 49736
Relevant Lines: 90805

💛 - 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.

Linter is unhappy, otherwise LGTM 🎉

Copy link
Contributor

@ffranr ffranr left a 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?

Comment on lines 32 to 42
bidPrices map[string]rfqmath.BigIntFixedPoint
askPrices map[string]rfqmath.BigIntFixedPoint
Copy link
Contributor

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).

@GeorgeTsagk
Copy link
Member Author

Thanks for the feedback @ffranr , addressed your comments
There's a leftover lint error which originates from base branch, will rebase on the updated base branch before merging so that will eventually go away

Copy link
Contributor

@ffranr ffranr left a 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.

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-negotiation-groupkey branch from 269ad4b to 7c660bd Compare March 4, 2025 14:23
@GeorgeTsagk GeorgeTsagk force-pushed the itest-oracle-harness branch from 3351424 to 80294e4 Compare March 4, 2025 14:53
@guggero
Copy link
Member

guggero commented Mar 4, 2025

I think you can use asset.Specifier as a map key. I think all fields are comparable.

We should be careful when comparing fields that have a btcec.PublicKey. Even though its members are comparable, it's possible that the same point on a curve might be encoded in different ways (see the Normalize() method of the FieldVal member).

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-negotiation-groupkey branch from 7c660bd to 6cf7190 Compare March 10, 2025 15:12
Base automatically changed from rfq-negotiation-groupkey to main March 11, 2025 17:31
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.
@GeorgeTsagk GeorgeTsagk force-pushed the itest-oracle-harness branch from 80294e4 to 4048004 Compare March 11, 2025 18:02
@GeorgeTsagk GeorgeTsagk enabled auto-merge March 11, 2025 18:03
@GeorgeTsagk GeorgeTsagk added this pull request to the merge queue Mar 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 11, 2025
@guggero guggero merged commit 4852be4 into main Mar 11, 2025
18 checks passed
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.

4 participants