Skip to content

Add itest oracle harness#1395

Merged
guggero merged 6 commits intomainfrom
itest-oracle-harness
Mar 11, 2025
Merged

Add itest oracle harness#1395
guggero merged 6 commits intomainfrom
itest-oracle-harness

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
Contributor

@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
Contributor

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
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Mar 11, 2025
@guggero guggero deleted the itest-oracle-harness branch March 24, 2025 20:18
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