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 price oracle proxy #1397

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sputn1ck
Copy link
Member

This PR adds a price oracle proxy to the rpcserver. The motivation behind is that an application dev might want to reuse the price oracle that is used with the connected tap daemon. Currently this would then mean that the application needs seperate connections for tapd and the price oracle. This simplyfies this.

Closes ##1356

AI generated summary

This pull request introduces a price oracle proxy feature to the system, along with necessary configurations and tests. The changes include adding new imports, configuration options, and implementing the price oracle proxy functionality in various parts of the codebase.

Key changes include:

Configuration and Imports:

  • Added priceoraclerpc import in multiple files to support the price oracle proxy feature. (config.go, itest/integration_test.go, itest/tapd_harness.go, perms/perms.go, rfq/mock.go, rfq/oracle.go, rpcserver.go, server.go) [1] [2] [3] [4] [5] [6] [7] [8]

Configuration Options:

  • Added AllowPublicPriceOracle to RPCConfig to control public access to the price oracle RPC endpoints. (config.go) [1] [2] [3]
  • Introduced ProxyPriceOracle in Config to proxy price requests to another price oracle. (config.go) [1] [2]

Implementation:

  • Implemented QueryAssetRates method in rpcServer to handle price oracle proxy requests. (rpcserver.go)
  • Registered the PriceOracleServer with the gRPC server. (rpcserver.go)

Testing:

  • Added testPriceOracleProxy to verify the functionality of the price oracle proxy. (itest/integration_test.go)
  • Included the new test in the test cases list. (itest/test_list_on_test.go)
  • Defined a mock implementation for QueryAssetRates in MockPriceOracle. (rfq/mock.go)

Miscellaneous:

  • Added ErrPriceOracleUnimplemented to handle cases where the price oracle service is not implemented. (rpcserver.go)

These changes collectively introduce and test the price oracle proxy feature, ensuring it integrates seamlessly with the existing system.

@coveralls
Copy link

coveralls commented Feb 18, 2025

Pull Request Test Coverage Report for Build 13400087732

Details

  • 35 of 48 (72.92%) changed or added relevant lines in 7 files are covered.
  • 61 unchanged lines in 14 files lost coverage.
  • Overall coverage increased (+0.08%) to 54.538%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapcfg/server.go 3 4 75.0%
perms/perms.go 2 4 50.0%
rfq/oracle.go 0 3 0.0%
rpcserver.go 8 15 53.33%
Files with Coverage Reduction New Missed Lines %
commitment/tap.go 1 85.0%
asset/group_key.go 2 72.65%
fn/iter.go 2 62.07%
mssmt/compacted_tree.go 2 83.27%
tapchannel/aux_leaf_signer.go 2 43.08%
tapdb/mssmt.go 2 88.64%
tapdb/sqlc/mssmt.sql.go 2 52.98%
itest/multisig.go 3 97.86%
rfqmsg/records.go 3 71.2%
universe/base.go 3 79.25%
Totals Coverage Status
Change from base Build 13397714787: 0.08%
Covered Lines: 48714
Relevant Lines: 89321

💛 - Coveralls

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.

(Summary of some points we discussed in chat:)

Instead of mirroring the price oracle's RPC endpoints, why not simply pass along its macaroon? We could add an RPC endpoint to tapd that returns the host, certificate, and macaroon of the price oracle service. Additionally, we could request a new sub-macaroon from the price oracle to maintain separated permissions.

Another consideration: if the client requires real, locally network-aware prices—that is, prices that counterparty edge nodes will accept—then we need to obtain that price through tapd and its RFQ process.

With that in mind, rather than duplicating the price oracle's RPC endpoint here, we could introduce a new tapd RPC endpoint. Initially, this endpoint would call the price oracle directly. However, in the future, it could leverage the RFQ system to retrieve a more accurate price across edge node peers.

This approach would also factor in asset illiquidity. We could name the new endpoint QueryBestAssetRate, or something similar.

@Roasbeef
Copy link
Member

We could add an RPC endpoint to tapd that returns the host, certificate, and macaroon of the price oracle service. Additionally, we could request a new sub-macaroon from the price oracle to maintain separated permissions.

I think that's a major isolation leak. We should make minimal assumptions about resource or authentication sharing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants