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

examples: add TLS support to basic price oracle service #1140

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Oct 3, 2024

This commit adds basic TLS support to the example price oracle service. When connecting to the RPC price oracle, Tapd now assumes TLS. TLS certificate authentication is skipped by Tapd.

Simplify integration of the example price oracle with Tapd. (Hopefully helpful for @HannahMR )

@ffranr ffranr requested review from gijswijs and GeorgeTsagk October 3, 2024 15:18
@ffranr ffranr self-assigned this Oct 3, 2024
@dstadulis dstadulis added this to the v0.4.2 milestone Oct 3, 2024
@ffranr ffranr requested a review from HannahMR October 3, 2024 15:21
@coveralls
Copy link

coveralls commented Oct 3, 2024

Pull Request Test Coverage Report for Build 11217147728

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.001%) to 40.365%

Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
commitment/tap.go 2 84.43%
asset/asset.go 2 81.61%
tapgarden/caretaker.go 4 68.5%
universe/interface.go 12 50.22%
Totals Coverage Status
Change from base Build 11131738711: 0.001%
Covered Lines: 24254
Relevant Lines: 60086

💛 - Coveralls

Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉
I can still see some magic numbers in there, but that's expected for essentially a mock. But I just wanted to flag it.

Also, this server now only works with TLS. That's what we want, right?

Made a nit comment, but nothing that would block merging.

docs/examples/basic-price-oracle/main.go Outdated Show resolved Hide resolved
This commit adds basic TLS support to the example price oracle service.
When connecting to the RPC price oracle, Tapd now assumes TLS.
TLS certificate authentication is skipped by Tapd.
@ffranr
Copy link
Contributor Author

ffranr commented Oct 3, 2024

Also, this server now only works with TLS. That's what we want, right?

@gijswijs yes, I think it will do for now. And tapd expects that right now.

@dstadulis dstadulis requested review from gijswijs and removed request for GeorgeTsagk October 3, 2024 17:13
@HannahMR
Copy link

HannahMR commented Oct 4, 2024

This does solve the communication error between litd and the oracle in my testing.

@ffranr ffranr requested a review from guggero October 7, 2024 12:20
@ffranr
Copy link
Contributor Author

ffranr commented Oct 7, 2024

Also, this server now only works with TLS. That's what we want, right?

That's fine IMO. We just want to make sure that it can work with tapd out of the box.

@ffranr ffranr force-pushed the add-tls-support-to-example-price-oracle branch from 2263a38 to 967415b Compare October 7, 2024 13:12
@ffranr
Copy link
Contributor Author

ffranr commented Oct 7, 2024

I've added logging to this and the format is sub-optimal. So I will improve before merge.

Compare subject asset to supported asset using both string and byte
slice instead of just string.
@ffranr ffranr force-pushed the add-tls-support-to-example-price-oracle branch from 66c0cae to 2d0b4a4 Compare October 7, 2024 13:58
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐄

@Roasbeef Roasbeef merged commit 7ebb640 into main Oct 7, 2024
16 of 18 checks passed
@guggero guggero deleted the add-tls-support-to-example-price-oracle branch October 7, 2024 16:04
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.

6 participants