Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price #228

Merged
merged 4 commits into from
May 19, 2020

Conversation

kimpers
Copy link
Contributor

@kimpers kimpers commented May 18, 2020

Description

This fixes #227.

When integrating with the /price endpoints I noticed that the data returned back is very sparse compared to the quote endpoint. I also noticed that two endpoints return different data, so I tried to unify to the point it makes sense.

For us, it would be very helpful to be able to derive the pair from the response data so we don't have to keep track of that locally in the global state.

Testing Instructions

TODO:

  • Test through on staging once the format is settled

Checklist

price: BigNumber;
buyAmount: BigNumber;
sellAmount: BigNumber;
sellTokenAddress: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the token address to be in line with the quote endpoint. However, the prices endpoint returns symbols.

Either symbol or token address would work as long as it's consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly.

It would be nice to have consistency across the board, if we go with token address here maybe we should add it there as well or vice versa.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it should be consistently address since we cannot guarantee symbol.

@kimpers kimpers changed the title Unify the response data from /swap/v0/price and /meta_transaction/v0/price fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price May 18, 2020
@kimpers kimpers changed the title fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price [WIP] May 18, 2020
@fragosti
Copy link
Contributor

@kimpers can you create a PR in the documentation as mentioned in https://github.com/0xProject/0x-api/blob/master/PULL_REQUEST_TEMPLATE.md

src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@dekz dekz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @kimpers.

price: BigNumber;
buyAmount: BigNumber;
sellAmount: BigNumber;
sellTokenAddress: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sellAmount,
buyAmount,
buyAmount: makerAssetAmount,
sellAmount: totalTakerAssetAmount,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that the metatx endpoint would only return the argument you pass to it, whereas the swap endpoint always returns buyAmount and sellAmount.

@kimpers
Copy link
Contributor Author

kimpers commented May 19, 2020

deploy staging

@kimpers
Copy link
Contributor Author

kimpers commented May 19, 2020

@kimpers can you create a PR in the documentation as mentioned in https://github.com/0xProject/0x-api/blob/master/PULL_REQUEST_TEMPLATE.md

Implemented in 0xProject/website#298.

Additionally, the linting is now failing due to CHANGELOG. Should I manually add an entry to the file or what is the process for this?

@kimpers kimpers changed the title fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price [WIP] fix: unify the response data from /swap/v0/price and /meta_transaction/v0/price May 19, 2020
@fragosti
Copy link
Contributor

@kimpers yes sorry about the changelog. Can you modify the prettier glob pattern in package.json to just ignore the changelog? Humans don't touch it anyway.

@kimpers
Copy link
Contributor Author

kimpers commented May 19, 2020

@kimpers yes sorry about the changelog. Can you modify the prettier glob pattern in package.json to just ignore the changelog? Humans don't touch it anyway.

Fixed 👍

@fragosti
Copy link
Contributor

@kimpers oh I just did #234

@kimpers kimpers force-pushed the kim/unify-interface-of-price-endpoints branch from dea962c to 68699f8 Compare May 19, 2020 17:29
@kimpers
Copy link
Contributor Author

kimpers commented May 19, 2020

@kimpers oh I just did #234

I see 👍 , dropped the commit now 😄

@kimpers kimpers force-pushed the kim/unify-interface-of-price-endpoints branch from 68699f8 to d06c39c Compare May 19, 2020 17:31
Copy link
Contributor

@fragosti fragosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I honestly didn't realize this was modifying the swap/v0/price endpoint. I think this is for RFQT only, and maybe that is why it is not documented. Seems confusing to have both /quote and /price if you don't know about RFQT.
So maybe I was wrong about documenting this, my apologies.

price: BigNumber;
buyAmount: BigNumber;
sellAmount: BigNumber;
sellTokenAddress: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it should be consistently address since we cannot guarantee symbol.

@kimpers kimpers merged commit 62f3fae into master May 19, 2020
@kimpers kimpers deleted the kim/unify-interface-of-price-endpoints branch May 19, 2020 18:30
github-actions bot pushed a commit that referenced this pull request May 25, 2020
# [1.5.0](v1.4.0...v1.5.0) (2020-05-25)

### Bug Fixes

* unify the response data from /swap/v0/price and /meta_transaction/v0/price ([#228](#228)) ([62f3fae](62f3fae))

### Features

* Add Prometheus Monitoring ([#222](#222)) ([5a51add](5a51add))
* added an epochs/n endpoint to get info on an arbitrary epoch ([#230](#230)) ([68ec159](68ec159))
* lower default slippage percentage to 1% ([#238](#238)) ([c7ec0ff](c7ec0ff))
* MetaTxn add signer heartbeat and status ([#236](#236)) ([3a11867](3a11867))
* set default skip RFQt buy requests to false ([#232](#232)) ([a5d7a1c](a5d7a1c))
@github-actions
Copy link

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Different response interface for /swap/v0/price and /meta_transaction/v0/price
4 participants