feat: Rename oracle to slinky, refactor mm provider factory #272

Eric-Warehime · 2024-03-25T05:20:10Z

This PR renames ./build/oracle => ./build/slinky.

It also refactors the marketmap provider factory to be more in line with the other factories (websocket and API). The idea being that once we remove the aggregator bit, we can get rid of all of the special casing for dYdX and rely on the factory to construct the proper marketmap provider for any chain we support.

I've pointed this PR at the eric/convert-everything-to-cobra branch since it made sense to change these in the context of the cobra refactor.

codecov · 2024-03-25T05:26:54Z

Codecov Report

Attention: Patch coverage is 14.70588% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 62.34%. Comparing base (0787b25) to head (f4c871e).
Report is 2 commits behind head on main.

❗ Current head f4c871e differs from pull request most recent head db63e31. Consider uploading reports for the commit db63e31 to get more accurate results

Files	Patch %	Lines
cmd/slinky/main.go	0.00%	30 Missing ⚠️
cmd/slinky-config/main.go	0.00%	26 Missing ⚠️
providers/factories/oracle/marketmap.go	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
+ Coverage   62.11%   62.34%   +0.22%     
==========================================
  Files         215      215              
  Lines        9503     9475      -28     
==========================================
+ Hits         5903     5907       +4     
+ Misses       3055     3023      -32     
  Partials      545      545

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aljo242 · 2024-03-25T13:40:55Z

providers/factories/oracle/marketmap.go

+		apiDataHandler, err = dydx.NewAPIHandler(cfg.API)
+		ids = []types.Chain{{ChainID: dydx.ChainID}}
+	default:
+		return nil, fmt.Errorf("unknown provider: %s", cfg.Name)


This case should return the default marketmap API handler that hits x/marketemap right?

or is the idea that we'll fill this info out later after the refactor?

davidterpay · 2024-03-25T14:10:58Z

cmd/slinky/main.go

@@ -132,6 +132,10 @@ func runOracle() error {
 		orchestrator.WithMarketMap(marketCfg),
 		orchestrator.WithPriceAPIQueryHandlerFactory(oraclefactory.APIQueryHandlerFactory),             // Replace with custom API query handler factory.
 		orchestrator.WithPriceWebSocketQueryHandlerFactory(oraclefactory.WebSocketQueryHandlerFactory), // Replace with custom websocket query handler factory.
+		orchestrator.WithMarketMapperFactory(oraclefactory.MarketMapProviderFactory),


We need the marketmap provider factory to only be included in the dYdX options because it will either run the mm provider and update based on what is on dYdX main-net or it will fail to be constructed if the market map provider is not included in the oracle config. There is probably a cleaner way with dealing how to run this based per chain and whether the mm provider is desired. But I'm assuming this isnt desired behavior.

I'm confused why this even needs to be an option? Shouldn't we be able to have casing logic like websocket / api, and have the default be a nop updater (no mm-provider? Or a mm provider that never updates)

This is an option that lets the orchestrator know how to construct a market map provider if one is included in the oracle.json. Reading through this again I think this looks good.

The factories take care of casing based on the API/Websocket implementation

davidterpay

given that the mm provider is only going to be utilized by dYdX for the time being, we need to move back into dYdXOptions

…r-mmfactory

Eric-Warehime · 2024-03-25T22:23:53Z

providers/factories/oracle/marketmap.go

+	switch cfg.Name {
+	case dydx.Name:
+		apiDataHandler, err = dydx.NewAPIHandler(cfg.API)
+		ids = []types.Chain{{ChainID: dydx.ChainID}}


I'm not sure how this is used right now, but it feels like we don't really need the chain-id as long as we point the mm provider at the correct node?

ya, how would our configs work if we have a market-map for multiple chains? Have we even resolved this?

Yea this is currently hard coded based on the desired chain but it doesnt really matter since all you have to do is point it to the correct URL. The providers were purposfully designed to assume you have inputs when constructing URLs, but in the future we can probably make it allow static or no entries.

Eric-Warehime · 2024-03-25T23:50:58Z

Approach I ended up with:

Ignores aggregation type entirely. Run the full aggregation paths validation on all configs we get.
Update CLIs as follows:
Config is pretty much as before.

slinky-config --chain "" -- node-http-url ""

Slinky requires a valid oracle.json path. Has optional flags, market-config-path or market-config-update-path which are mutually exclusive.
market-config-path is the path to the market.json to start the system with (same as we currently have)--though that will get updated if you have a market map provider present.
market-config-update-path is a place where we will write market updates in line with what the sidecar is doing. Sidecar initially bootstraps with an empty config and writes updates to this path.
Specifying neither of these will initialize with an empty market map and updates will never be written to disk.

slinky --oracle-config-path "./oracle.json"
slinky --oracle-config-path "./oracle.json" --market-config-path ./market.json
slinky --oracle-config-path "./oracle.json" --market-config-update-path ./new-file.json

nivasan1 · 2024-03-26T00:36:32Z

cmd/slinky/main.go

@@ -132,6 +132,10 @@ func runOracle() error {
 		orchestrator.WithMarketMap(marketCfg),
 		orchestrator.WithPriceAPIQueryHandlerFactory(oraclefactory.APIQueryHandlerFactory),             // Replace with custom API query handler factory.
 		orchestrator.WithPriceWebSocketQueryHandlerFactory(oraclefactory.WebSocketQueryHandlerFactory), // Replace with custom websocket query handler factory.
+		orchestrator.WithMarketMapperFactory(oraclefactory.MarketMapProviderFactory),


I'm confused why this even needs to be an option? Shouldn't we be able to have casing logic like websocket / api, and have the default be a nop updater (no mm-provider? Or a mm provider that never updates)

nivasan1 · 2024-03-26T00:37:00Z

cmd/slinky/main.go

@@ -129,29 +123,33 @@ func runOracle() error {
 	}

 	metrics := oraclemetrics.NewMetricsFromConfig(cfg.Metrics)
+	aggregator, err := oraclemath.NewMedianAggregator(


I thought we agreed to have a single aggregation strat in the oracle (the dydx one)

This is the dydx aggregation.

nivasan1 · 2024-03-26T00:43:26Z

providers/factories/oracle/marketmap.go

+	switch cfg.Name {
+	case dydx.Name:
+		apiDataHandler, err = dydx.NewAPIHandler(cfg.API)
+		ids = []types.Chain{{ChainID: dydx.ChainID}}


ya, how would our configs work if we have a market-map for multiple chains? Have we even resolved this?

nivasan1 · 2024-03-26T01:34:31Z

cmd/slinky-config/main.go

+				tickersToPaths[tickerStr] = mmtypes.Paths{}
+			}
+			paths := tickersToPaths[tickerStr].Paths
+			paths = append(paths, mmtypes.Path{Operations: []mmtypes.Operation{


We're medianizing over all paths, no? This would give extra weight to the raw feed if it alr existed?

It's just a pure median over all feeds with optional 1 hop inversion.

nivasan1 · 2024-03-26T01:38:01Z

cmd/slinky-config/main.go

 	)
 	rootCmd.Flags().StringVarP(
 		&nodeURL,
 		"node-http-url",
 		"",
 		"",
-		"http endpoint of the cosmos sdk node corresponding to the chain id (typically something like localhost:1317). this should only be specified if required by the chain.",
+		"Http endpoint of the cosmos sdk node corresponding to the chain (typically localhost:1317 or a remote API). This should only be specified if required by the chain.",
 	)
 	rootCmd.Flags().StringVarP(
 		&host,
 		"host",
 		"",
 		"0.0.0.0",


Are we checking that the default value isn't used if no chain is given here?

This variable is only used if dydx chain is specified otherwise it's ignored at the moment.

nivasan1 · 2024-03-26T01:39:06Z

pkg/math/oracle/median.go

@@ -38,11 +38,6 @@ func NewMedianAggregator(
 		return nil, err
 	}

-	if cfg.AggregationType != mmtypes.AggregationType_INDEX_PRICE_AGGREGATION {


Is there a reason we can't remove the agg-type from the config, now?

I left it since we're rewriting all of the mm stuff and deleting all the proto and tests didn't seem worth it at this point.

What is the reason we need to remove this rn? From my perspective I put this in the case where operators accidentally use the wrong market.json file. this was an easy way to make sure things were configured correctly. the dYdX market params API constructs the market.json to include the field to be set correctly.

Supporting multiple aggregation types seems like a really bad idea in general. It will make it even harder to reason about how a price was determined and introduce potential for multi-modal behavior in our on chain prices based on a bug in one place but not another.

I also removed it because it was the one stateful piece of validation that was preventing us from bootstrapping the oracle without a market.json config.

yea agree, multiple not gud. im fine making this change now

oracle/constants/chains.go

Co-authored-by: David Terpay <[email protected]>

…r-mmfactory

davidterpay

We're going to need to update the local market.json if we want to enforce that the single aggregation type is utilized otherwise any build is going to fail on start.

Eric-Warehime · 2024-03-26T15:25:25Z

We're going to need to update the local market.json

@davidterpay It's updated in this PR

davidterpay · 2024-03-26T15:29:21Z

cmd/slinky-config/main.go

 	)
 	rootCmd.Flags().DurationVarP(
 		&updateInterval,
 		"update-interval",
 		"",
 		500*time.Millisecond,
-		"interval at which the oracle will update the prices. this should be set to the interval desired by the chain.",
+		"Interval at which the oracle will update the prices. This should be set to the interval desired by the chain.",


I would make sure that this is set to 500 milliseconds

Eric-Warehime added 4 commits March 22, 2024 16:56

Convert all commands to cobra

7d8d191

Fix lint

0b0e899

Merge branch 'main' into eric/convert-everything-to-cobra

adb39d8

Rename oracle to slinky, refactor mm provider factory

0fcc9dd

Eric-Warehime requested review from davidterpay, nivasan1 and aljo242 as code owners March 25, 2024 05:20

aljo242 reviewed Mar 25, 2024

View reviewed changes

davidterpay reviewed Mar 25, 2024

View reviewed changes

Base automatically changed from eric/convert-everything-to-cobra to main March 25, 2024 15:26

Eric-Warehime added 6 commits March 25, 2024 08:30

Merge remote-tracking branch 'origin' into eric/rename-slinky-refacto…

c50a833

…r-mmfactory

Update aggregation stuff

c1cd3ee

Merge remote-tracking branch 'origin' into eric/rename-slinky-refacto…

5960108

…r-mmfactory

Fix metrics stuff I broke in merge

ebdb2aa

Remove chain arg from slinky

6d8b7fb

Change names, flags

adeecd2

Eric-Warehime commented Mar 25, 2024

View reviewed changes

Eric-Warehime added 4 commits March 25, 2024 15:38

Do validate

279db59

Fix path tests

d5e7d3d

Fix keeper tests

7c2230f

Fix integ tests

7a8f3b9

Eric-Warehime added 4 commits March 25, 2024 17:41

Generate paths for default configs

0da58f2

Fix lint fumpt

a752c03

Fix integ tests

77faa08

Provide paths everywhere

67614b2

aljo242 approved these changes Mar 26, 2024

View reviewed changes

Fix slashing tests

a62d7d6

nivasan1 reviewed Mar 26, 2024

View reviewed changes

update

f19b85e

Eric-Warehime requested a review from davidterpay March 26, 2024 02:31

davidterpay reviewed Mar 26, 2024

View reviewed changes

oracle/constants/chains.go Outdated Show resolved Hide resolved

Eric-Warehime and others added 2 commits March 26, 2024 08:20

Update oracle/constants/chains.go

ea8f03b

Co-authored-by: David Terpay <[email protected]>

Merge remote-tracking branch 'origin' into eric/rename-slinky-refacto…

f4c871e

…r-mmfactory

davidterpay reviewed Mar 26, 2024

View reviewed changes

davidterpay approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'main' into eric/rename-slinky-refactor-mmfactory

db63e31

Eric-Warehime merged commit 2f64382 into main Mar 26, 2024
11 checks passed

Eric-Warehime deleted the eric/rename-slinky-refactor-mmfactory branch March 26, 2024 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rename oracle to slinky, refactor mm provider factory #272

feat: Rename oracle to slinky, refactor mm provider factory #272

Eric-Warehime commented Mar 25, 2024

codecov bot commented Mar 25, 2024 •

edited

Loading

aljo242 Mar 25, 2024

davidterpay Mar 25, 2024

nivasan1 Mar 26, 2024

davidterpay Mar 26, 2024

davidterpay Mar 26, 2024

davidterpay left a comment

Eric-Warehime Mar 25, 2024

nivasan1 Mar 26, 2024

davidterpay Mar 26, 2024

Eric-Warehime commented Mar 25, 2024

nivasan1 Mar 26, 2024

nivasan1 Mar 26, 2024

Eric-Warehime Mar 26, 2024

nivasan1 Mar 26, 2024

nivasan1 Mar 26, 2024

Eric-Warehime Mar 26, 2024

nivasan1 Mar 26, 2024

Eric-Warehime Mar 26, 2024

nivasan1 Mar 26, 2024

Eric-Warehime Mar 26, 2024

davidterpay Mar 26, 2024

Eric-Warehime Mar 26, 2024

davidterpay Mar 26, 2024

davidterpay left a comment

Eric-Warehime commented Mar 26, 2024

davidterpay Mar 26, 2024

feat: Rename oracle to slinky, refactor mm provider factory #272

feat: Rename oracle to slinky, refactor mm provider factory #272

Conversation

Eric-Warehime commented Mar 25, 2024

codecov bot commented Mar 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Warehime commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay left a comment

Choose a reason for hiding this comment

Eric-Warehime commented Mar 26, 2024

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2024 •

edited

Loading