This repository has been archived by the owner on Jul 9, 2021. It is now read-only.
[asset-buyer][instant] Fix incorrect token prices for non 18-decimal tokens #1252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BMillman19
force-pushed
the
fix/asset-buyer/price-per-token
branch
from
928424a
to
b65b410
Compare
BMillman19
changed the title
BMillman19
force-pushed
the
fix/asset-buyer/price-per-token
branch
from
b65b410
to
e8afc66
Compare
| @@ -14,13 +14,15 @@ export interface LatestBuyQuoteOrderDetailsProps {} | |||
|
|
|||
| interface ConnectedState { | |||
| buyQuoteInfo?: BuyQuoteInfo; | |||
| selectedAssetAmount?: BigNumber; | |||
There was a problem hiding this comment.
Per convo, use word "unit" to describe selected asest amount
| const assetEthBaseUnitAmount = buyQuoteAccessor.assetEthAmount(); | ||
| const feeEthBaseUnitAmount = buyQuoteAccessor.feeEthAmount(); | ||
| const totalEthBaseUnitAmount = buyQuoteAccessor.totalEthAmount(); | ||
| const perAssetUnitEthBaseUnitAmount = |
There was a problem hiding this comment.
this var name is descriptive but pretty long and gnarly, could we just call it something like pricePerTokenEth?
…amed selectedAssetUnitAmount
BMillman19
force-pushed
the
fix/asset-buyer/price-per-token
branch
from
80e3773
to
5527de6
Compare
packages/asset-buyer/CHANGELOG.json
Outdated
| @@ -1,4 +1,13 @@ | |||
| [ | |||
| { | |||
| "version": "2.3.0", | |||
There was a problem hiding this comment.
Should this be a full version bump (3)?
fragosti
approved these changes
Nov 13, 2018
* development: Publish Updated CHANGELOGS Fix a bug when undefined was been tried to convert to an array feat(instant): add extra asset metadata chore: fix linter error Improve logo spacing fix: lowercase supplied address before comparing with derived addresses (which are not checksummed) Remove unused instance variable update yarn.lock Increase logo size Replace remaining scroll-links with Link component Adjust paddin Fix menuItem background colors depending on the context Remove "Home" menu item, instead make different parts of logo link to different sections of the website Rename tutorial to match verb structure chore: Make `External exports` clickable on sidebar style: reduce border size on version dropdown style: make line-height of sidebar title 26px, make sure still bottom aligned with version picker Update yarn.lock style: remove small gap under topbar
steveklebanoff
approved these changes
Nov 13, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
AssetBuyerto return information about total eth spent on the target asset instead of per-unit token prices.AssetBuyerto include eth spent on zrx tokens to fund takerFees as part of thefeeEthAmountreturned bygetBuyQuoteAsync. Previously this cost was being reported as part of the base amount being paid for an asset and thefeeEthAmountrepresented only the affiliate fee.Instantto calculate per-unit token prices using the amount of units that is provided by the userInstantto be clearer what amounts represent normal units vs base unitsTesting instructions
Unit tests
Types of changes
Checklist:
[WIP]if necessary.[sol-cov] Fixed bug.