feat: Crescent and Osmosis LPs tokens prices #152

Pitasi · 2022-06-08T15:43:45Z

Overview of this PR:

I added two "ports": Crescent and Osmosis specific gRPC interfaces
We implement different "strategies" for retrieving a list of pools, the returned pools implements a common interface. As you can imagine the osmosis strategy uses the osmosis grpc client, same for crescent.
Things can go wrong: a grpc endpoint might be offline (whole strategy fails), or a single pool in the list might cause some troubles (e.g. we don't have a price for the denoms inside that pool). These errors should not prevent the entire list to be generated, hence I created an options pkg that is basically a pair <Value, Error> safe to be json marshalled.

Future work:

DatabaseDenomer expose a method to get a denom by its name. Its implementation fetches the whole list of denoms first, and the returns the matching denom if any. It should cache that list (at least for a few second) so that multiple close invocations don't result in multiple database queries.

lib/options/options.go

Pitasi · 2022-06-10T12:51:20Z

I did a bunch of rewrites of the code in this PR, I'll squash and reorganize the commits at the end so you can review it easier 😅

tbruyelle · 2022-06-10T12:56:04Z

I did a bunch of rewrites of the code in this PR, I'll squash and reorganize the commits at the end so you can review it easier sweat_smile

No problem I didn't really start since the PR is in draft.

The liquidity module conflicts with the new Crescent gRPC.

It basically holds a Value and an Error. This option type is json serializable.

tbruyelle · 2022-06-15T13:09:53Z

api/database/chains_test.go

@@ -380,3 +380,9 @@ func (s *TestSuite) TestChainsOnlineStatuses() {
 		})
 	}
 }
+
+func (s *TestSuite) TestDenoms() {
+	denoms, err := s.ctx.Router.DB.Denoms(context.Background())


With this setup, you can expect one denom :

err := s.ctx.CnsDB.AddChain(utils.ChainWithoutPublicEndpoints) require.NoError(t, err)

tbruyelle · 2022-06-15T13:16:07Z

cmd/api-server/main.go

-	app := usecase.NewApp(sdkServiceClients)
+	osmosisClient := usecase.NewOsmosisGrpcClient("osmosis:9090")
+
+	crescentClient := usecase.NewCrescentGrpcClient("crescent:9090")


IMO these GRPC clients shouldn't be located in the usecase package, because they don't contain business code. Moreover they can't be tested efficiently. I suggest to move them in a specific package like poclient, for instance grpcclient ?

In addition, it's weird to have both OsmosisClient interface and implementation in the same package

tbruyelle · 2022-06-15T13:19:32Z

usecase/usecase_test.go

@@ -20,6 +23,7 @@ func newApp(t *testing.T, setup func(mocks)) *usecase.App {
 		t:                 t,
 		sdkServiceClients: NewMockSDKServiceClients(ctrl),
 		sdkServiceClient:  NewMockSDKServiceClient(ctrl),
+		denomPricer:       NewMockDenomPricer(ctrl),


For the tests, you'll need to feed mocks.crescentClient and mocks.osmosisClient.

tbruyelle · 2022-06-15T13:23:17Z

usecase/prices.go

+type PriceOracle interface {
+	GetPrice(ctx context.Context, symbol string) (poclient.Price, error)
+}
+
+type DenomTracer interface {
+	DenomTrace(ctx context.Context, chainName, traceHash string) (tracelistener.IBCDenomTraceRow, error)
+}
+
+type Denomer interface {
+	Denom(ctx context.Context, denomName string) (cns.Denom, error)
+}


I assume you will move those interfaces in ports.go, or else you won't have the generated mocks.

tbruyelle · 2022-06-15T13:28:42Z

usecase/prices.go

+}
+
+func (d *DatabaseDenomer) Denom(ctx context.Context, denomName string) (cns.Denom, error) {
+	denoms, err := d.db.Denoms(ctx)


Typically usecase should never call the database directly like that, or else you will have to setup a local database for the test. What you need is a DB interface, like I started to work with in EmerisHQ/demeris-backend#777

demeris-api-server/usecase/ports.go

Lines 14 to 20 in eaed1f3

type DB interface {

Chains(ctx context.Context) ([]cns.Chain, error)

Balances(ctx context.Context, addresses []string) ([]tracelistener.BalanceRow, error)

Delegations(ctx context.Context, addresses []string) ([]database.DelegationResponse, error)

VerifiedDenoms(context.Context) (map[string]cns.DenomList, error)

DenomTrace(ctx context.Context, chain string, hash string) (tracelistener.IBCDenomTraceRow, error)

}

tbruyelle · 2022-06-15T15:08:03Z

usecase/prices.go

+	}
+}
+
+func (p *PriceOracleDenomPricer) DenomPrice(ctx context.Context, chainName, denomName string) (sdktypes.Dec, error) {


Because dependency injection is done manually with this pattern (once I tried to use wire but it was clearly over-engineered, so I stepped back), I tend to keep the number of services small. So typically here, I would have attached DenomPrice to App.

That said this is just a suggestion, if you prefer like that that's problably fine.

tbruyelle · 2022-06-15T15:30:59Z

usecase/lpools.go

+//     "osmosis"    -> [ {pool1}, {error pool2}, {pool3} ]
+//     "crescent"   -> [ {pool1}, {pool2}, {pool3} ]
+//     "chainx"     -> {error chainx}
+type PoolPricesResult map[StrategyID]options.O[[]options.O[PoolPrice]]


I'm mixed with the JSON output of this type.

First, I tend to never use a map for a type intended to be marshaled into JSON, because the order of keys isn't stable.

Secondly, despite I like the one-line usage of this new options package, I'm not fan of the value/error dichotomy once marshaled into JSON.

As a client of this endpoint, I have to choose to check the presence of value or error, and let's say I choose error, then I assume there's must be a value. It can be simpler I think. In addition, when there's an error, you can't add any additional information.

Suggestion : in case of error, we only add an error field embedded in the struct. The client just checks the presence of the error field. I tried to transform the data in the screenshot below with this different format (and I also removed the map) :

{ "pool_results": [ { "chain":"crescent", "pools": [ { "symbol": "pool8", "supply": "0", "price": "0" }, { "symbol": "pool9", "supply": "0", "price": "0", "error": "getting denom price: getting price for denom bCRE: symbol not found: BCREUSDT" } ] }, { "chain":"osmosis", "error":"error getting pool prices for strategy osmosis: cannot get pools, rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing dial tcp 10.64.208.2:9090: connect: connection refused\"" } ] }

WDYT?

github-actions bot added the patch label Jun 8, 2022

Pitasi force-pushed the feat/pool-prices branch 2 times, most recently from 9cc187a to 1411d1b Compare June 9, 2022 14:48

tbruyelle reviewed Jun 9, 2022

View reviewed changes

lib/options/options.go Outdated Show resolved Hide resolved

Pitasi force-pushed the feat/pool-prices branch from edc52f1 to 9557d8d Compare June 14, 2022 13:50

chore: cleanup unused cached pkg and old liquidity module

df7223f

The liquidity module conflicts with the new Crescent gRPC.

Pitasi force-pushed the feat/pool-prices branch from 7a56467 to 381b9c7 Compare June 14, 2022 15:17

Pitasi changed the title ~~wip: Osmosis LPs tokens prices~~ feat: Crescent and Osmosis LPs tokens prices Jun 14, 2022

Pitasi added 12 commits June 15, 2022 09:34

fix: use Int and Dec from cosmos-sdk pkg

7bf53b5

feat(poclient): use context in http requests

102c9f2

feat(database): add method to list all enabled denoms

0685939

feat(options): introduce an options pkg

11550e5

It basically holds a Value and an Error. This option type is json serializable.

feat: introduce DenomPricer port

5b8caee

feat: introduce ports for Crescent and Osmosis gRPC interfaces

4d8aba1

feat: add new ports to App struct

ae81aea

feat: add business logic for getting prices of the pools

25a1211

feat: introduce Osmosis and Crescent LP strategies

1e0f236

feat: add new /prices/pools endpoint

02103a9

feat: initialize new App dependencies

09a1f44

chore: go mod tidy

43df220

Pitasi force-pushed the feat/pool-prices branch from 381b9c7 to 43df220 Compare June 15, 2022 07:34

Pitasi marked this pull request as ready for review June 15, 2022 07:34

Pitasi requested a review from a team as a code owner June 15, 2022 07:34

Pitasi requested review from tbruyelle, jeronimoalbi and zarazan June 15, 2022 07:34

tbruyelle reviewed Jun 15, 2022

View reviewed changes

test: add basic tests for crescent strategy

5299a16

tbruyelle reviewed Jun 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Crescent and Osmosis LPs tokens prices #152

feat: Crescent and Osmosis LPs tokens prices #152

Pitasi commented Jun 8, 2022 •

edited

Loading

Pitasi commented Jun 10, 2022

tbruyelle commented Jun 10, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

tbruyelle Jun 15, 2022

	type DB interface {
	Chains(ctx context.Context) ([]cns.Chain, error)
	Balances(ctx context.Context, addresses []string) ([]tracelistener.BalanceRow, error)
	Delegations(ctx context.Context, addresses []string) ([]database.DelegationResponse, error)
	VerifiedDenoms(context.Context) (map[string]cns.DenomList, error)
	DenomTrace(ctx context.Context, chain string, hash string) (tracelistener.IBCDenomTraceRow, error)
	}

feat: Crescent and Osmosis LPs tokens prices #152

Are you sure you want to change the base?

feat: Crescent and Osmosis LPs tokens prices #152

Conversation

Pitasi commented Jun 8, 2022 • edited Loading

Pitasi commented Jun 10, 2022

tbruyelle commented Jun 10, 2022

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

tbruyelle Jun 15, 2022

Choose a reason for hiding this comment

Pitasi commented Jun 8, 2022 •

edited

Loading