feat: Add marketmap module/keeper #114
Conversation
Eric-Warehime
requested review from
davidterpay,
nivasan1 and
aljo242
as code owners
Eric-Warehime
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feat/marketmap #114 +/- ##
==================================================
- Coverage 75.08% 74.94% -0.15%
==================================================
Files 141 144 +3
Lines 5775 5838 +63
==================================================
+ Hits 4336 4375 +39
- Misses 1086 1102 +16
- Partials 353 361 +8 ☔ View full report in Codecov by Sentry. |
x/marketmap/keeper/keeper.go
Outdated
| cdc codec.BinaryCodec | ||
|
|
||
| // keeper dependencies | ||
| oracleKeeper types.OracleKeeper |
There was a problem hiding this comment.
I think we really should not be adding anything from x/oracle as a dependency into x/marketmap. My thinking was for x/marketmap to expose hooks in a pattern similar to how some other cosmos sdk modules do.
Something like AfterMarketMapUpdate() which can be used by x/oracle (or x/prices) to get a view of changes without adding a direct keeper dependency
There was a problem hiding this comment.
Could get around this via an interface + implementation for the x/oracle module?
There was a problem hiding this comment.
Removing oracleKeeper interface from deps here. Plan is to have a follow up PR which includes hooks.
x/marketmap/keeper/keeper.go
Outdated
| marketConfigs collections.Map[string, types.MarketConfig] | ||
| // aggregationConfigs is keyed by CurrencyPair string (BASE/QUOTE) and contains the PathsConfig used | ||
| // to do price aggregation for a given canonical Ticker | ||
| aggregationConfigs collections.Map[string, types.PathsConfig] |
There was a problem hiding this comment.
This is not in the scope of this PR, but I'm wondering if we could/should introduce a type alias
type TickerString stringso we can more easily reason about the types of keys we are using to index into maps throughout the codebase
There was a problem hiding this comment.
is it not possible to use the CurrencyPair obj. as they key to this Map?
There was a problem hiding this comment.
Yeah that seems like a great idea. 🚀
| // MarketProvider is the unique name used to key the MarketConfigs in the marketmap module. | ||
| // It is identical to the MarketConfig.Name property which is stored as the value in the Keeper.marketConfigs map. | ||
| type MarketProvider string | ||
|
|
||
| // TickerString is the key used to identify unique pairs of Base/Quote with corresponding PathsConfig objects--or in other words AggregationConfigs. | ||
| // The TickerString is identical to Slinky's CurrencyPair.String() output in that it is `Base` and `Quote` joined by `/` i.e. `$BASE/$QUOTE`. | ||
| type TickerString string |
There was a problem hiding this comment.
does it make sense to put these in ./pkg/types as common types?
Done--BLO-904 |
Adds marketmap module and keeper implementation.
I've adhered generally to the proto implementations that were committed to the feature branch, although I think a broader conversation should be had about how those are laid out (we can change that in a separate PR if we agree on a different data layout there).
I've written this initial implementation around the idea of 2 access patterns:
Managing them separately seems the most reasonable to me since it enables us to have a 2 phase operation in which we turn on new currency pair feeds in the MarketConfigs and subsequently enable the prices in the x/oracle module by creating an aggregation config.
I've left out validation for now--happy to add that logic and tests in if it's crucial for this PR, I think a lot of it already exists in the proto types package already.
BLO-879