search

gnosis / gp-v1-ui

16 stars 41 forks source link

Use new price endpoint #790

Closed anxolin closed 4 years ago

anxolin commented 4 years ago

@fleupold will provide the endpoint URL

This price, will provide a better estimate. Instead of suggesting the last price, it suggest one that takes into account the user balances and open orders, so the trade will be more likely to happen at this price

We'll replace the implementation of out price suggestion to use the new endpoint

fleupold commented 4 years ago

Mainnet: https://price-estimate-mainnet.dev.gnosisdev.com/ Rinekby: https://price-estimate-rinkeby.dev.gnosisdev.com/

API documentation can be found in the README (currently under review).

fleupold commented 4 years ago

Quick question regarding the API. I noticed the price is currently not accounting for decimals. E.g. if you query https://price-estimate-mainnet.dev.gnosisdev.com/price?base=4&quote=1&sell=100000000 you get something like 7492843635.060851 (which given that USDC has 6 decimals, means a price of 0.0074 USDC/ETH or 133 ETH/USDC).

I don't have the decimals natively available in the backend - I can query them from chain, but that would require a bit more calls/logic. Given that the frontend already knows the decimals for all tokens, would it make sense to return the expected buyAmount instead of a price?

So the query above would return 749284363506085100000000 (which means you are getting ~0.75 ETH for the 100USDC you are trying to sell).

anxolin commented 4 years ago

I think this, being a generic price feed for any app, it shouldn't expose the internals of the contracts. I would really suggest to take decimals into account.

If you want to make it easier you can get it from the graph. In one query you have all names, symbols, ids etc etc. Would if you want to allow in the future to use symbols and addresses for the token as the people is use to use in other exchange APIs.

My personal opinion is:

anxolin commented 4 years ago

You plan to have an API in the future with a rust, right? Probably at one moment, we will replace one thing with the other.

It would be good to define the endpoint with a name that would allow a progresive transition in the future.

/api/v1/prices/<token1>-<token2>?amount=

This way, you'll be able to add other endpoints with rust implementation without taking this down /api/v1/other-endpoint

and evolpe the API when there's breaking changes: /api/v2/other-endpoint

fleupold commented 4 years ago

I think this, being a generic price feed for any app, it shouldn't expose the internals of the contracts

I'd argue the main use case for someone that asks for a price estimation is so that they can place an order. Since the order placement requires buy and sellAmount in the token's atomic quantity (wei for ETH, micro for USDC), I think it makes sense if the API returns the amounts.

Regarding a versioned URL, wouldn't the rust service bind another host/port and thus likely be a separate URL (unless we want to plug some fancy routing in between)?

anxolin commented 4 years ago

It's possible to have a reverse proxy, for that, one easy setup devops it's use to do is using nginx for this, and I think AWS have products for this

I'd argue the main use case for someone that asks for a price estimation is so that they can place an order...

Not 100% sure, it's possible. I feel more strong about the price than the amount. But for the amount, let's ask around. I feel people might use this to compare prices between different exchanges, or just show the price for a given input. The users enter the inputs in units, not weis.

fleupold commented 4 years ago

The users enter the inputs in units, not weis.

The user does normally not interact with an API directly, but usually via some frontend that already has more information regarding the tokens (e.g. the tokenSymbol <-> tokenID mapping, and thus likely also the decimals to do the conversion). Otherwise we should probably expose the base/quote token as the symbol string and not as some contract specific ID. This would then pose the question on how to handle symbol collisions if people list arbitrary ERC20s.

We could query by contract address but that seems as cryptic as the ID and would further increase the amount of queries we have to do in the backend when all we really need is to return the predicted buyAmount for a given sellAmount and token pair.

josojo commented 4 years ago

I don't have a strong opinion, both ways are fine. Intuitively, I would lean to that:

I think this, being a generic price feed for any app, it shouldn't expose the internals of the contracts. I would really suggest taking decimals into account.

Especially, as a developer using the API, from time to time I want to check the numbers for the correctness and then it feels easier if the decimals are taken account.

Otherwise we should probably expose the base/quote token as the symbol string and not as some contract specific ID. This would then pose the question on how to handle symbol collisions if people list arbitrary ERC20s.

Here, I guess we just have to take the practical approach and use the tokenId, just because otherwise, there is this risk that you are mentioning. Although it is not user friendly, for sure.

anxolin commented 4 years ago

Here, I guess we just have to take the practical approach and use the tokenId, just because otherwise, there is this risk that you are mentioning. Although it is not user friendly, for sure.

Yes, agree we should as a first version go with IDs, although I do believe we can and should support symbols in the future. Very likely we will provide already a functionality for this, because we have similar needs in the web, telegram bot or the CLI. An implementation of this would use the intersection of ALL the listed tokens with a trusted TCR. We are considering already using Kleros

We could query by contract address but that seems as cryptic as the ID and would further increase the amount of queries we have to do in the backend when all we really need is to return the predicted buyAmount for a given sellAmount and token pair.

Well, I agree that ID it's a must. But I think we should allow to use address too, at least at a later point if that overcomplicates the launch. I don't consider address cryptic at all. For users this will be a super useful option, I'm sure. Just by pasting addresses they already know about they will be able to get the price/book with no need to keep track of the ids for each token.

I don't agree that this would increase the amount of queries, you can use strong caches for keeping track of the ids, or any information of the token. Tokens don't change.