[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131

fragosti · 2018-10-11T01:37:07Z

Description

Straight port of features:

Abstraction for fetching async data, and fetching ETH-USD price from coinbase.
Connecting InstantHeading and BuyButton to the redux store.
Instantiate provider, asset buyer, web3wrapper etc...
Display the buy quote and allow the buy button to execute the buy quote.
Add notion of dynamic assetData and assetMetaData.
Add some utils for formatting eth price and eth-usd price.
Populate the order details section.
Add debouncing to fetches that result from input changes.
Make redux actions type-safe.

I also changed some types but then ended up not needing the change... but still seems like a good change? Basically we should control all unions of different asset data types, such that the build fails if we add a type to that union, assuming we are using a switch statement in the implementation of the function that is meant to handle all asset data types.

Testing instructions

Types of changes

Checklist:

Prefix PR title with [WIP] if necessary.
Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

fragosti · 2018-10-11T01:38:01Z

packages/instant/src/components/instant_heading.tsx

@@ -26,18 +57,18 @@ export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = pro
                <SelectedAssetAmountInput fontSize="45px" />
                <Container display="inline-block" marginLeft="10px">
                    <Text fontSize="45px" fontColor={ColorOption.white} textTransform="uppercase">
-                        rep
+                        zrx


Remove hardcoded zrx values

fragosti · 2018-10-11T01:38:17Z

packages/instant/src/constants.ts

@@ -0,0 +1,4 @@
+export const sraApiUrl = 'https://api.radarrelay.com/0x/v2/';
+export const zrxContractAddress = '0xe41d2489571d322189246dafa5ebde1f4699f498';
+export const zrxDecimals = 18;


remove hard-coded zrx values

fragosti · 2018-10-11T01:38:47Z

packages/instant/src/containers/selected_asset_amount_input.ts

+        // invalidate the last buy quote.
+        dispatch({ type: ActionTypes.UPDATE_LATEST_BUY_QUOTE, data: undefined });
+        // reset our buy state
+        dispatch({ type: ActionTypes.UPDATE_SELECTED_ASSET_BUY_STATE, data: AsyncProcessState.NONE });


create action creators? would allow us to have typed actions...

fragosti · 2018-10-11T01:39:11Z

packages/instant/src/redux/async_data.ts

+    fetchAndDispatchToStore: async () => {
+        let ethUsdPriceStr = '0';
+        try {
+            ethUsdPriceStr = await coinbaseApi.getEthUsdPrice();


coinbase API call should return BigNumber

fragosti · 2018-10-11T01:39:43Z

packages/instant/src/redux/reducer.ts

 import { BigNumber } from '@0xproject/utils';
 import * as _ from 'lodash';

-import { Action, ActionTypes } from '../types';
+import { Action, ActionTypes, AsyncProcessState } from '../types';

 export interface State {


add state for Provider, and other things required to remove hard-coded values.

Not going to do this for now. Should be done when we implement different wallets.

…into feature/instant/move-features-over-from-zrx-buyer

BMillman19 · 2018-10-15T20:34:46Z

packages/instant/src/components/amount_input.tsx

@@ -10,10 +10,13 @@ export interface AmountInputProps {
    fontColor?: ColorOption;
    fontSize?: string;
    value?: BigNumber;
-    onChange?: (value?: BigNumber) => void;
+    onChange: (value?: BigNumber) => void;


is value undefined if the input is empty?

yep! This is what made most sense to me.

BMillman19 · 2018-10-15T20:35:19Z

packages/instant/src/components/asset_amount_input.tsx

+
+export interface AssetAmountInputProps extends AmountInputProps {
+    assetData?: string;
+    onChange: (value?: BigNumber, assetData?: string) => void;


are we ever going to have a case where we have a value but not an assetData?

In practice no, but those two things can be undefined independently and that case needs to be handled somewhere. The other option is to not render something, but in the event that we are rendering, we need to handle it here.

BMillman19 · 2018-10-15T20:37:13Z

packages/instant/src/components/asset_amount_input.tsx

+            </Container>
+        );
+    }
+    private readonly _getLabel = (): string => {


maybe this can have a clearer name like _getAssetSymbolLabel

BMillman19 · 2018-10-15T20:39:11Z

packages/instant/src/components/asset_amount_input.tsx

+        const { assetData, onChange, ...rest } = this.props;
+        return (
+            <Container>
+                <AmountInput {...rest} onChange={this._handleChange} />


this is a design / polish nit, I think we want the text in this input to be right justified so that if you have typed just '5' then the character '5' is close to the symbol label

Will do in a future PR (added to my list)

BMillman19 · 2018-10-15T20:44:18Z

packages/instant/src/components/buy_button.tsx

-        </Button>
-    </Container>
-);
+const boundNoop = _.noop.bind(_);


should we extract this out into a util or something since we are using it in multiple components? Also do we necessarily need to bind here? I suppose the linter may complain?

The linter complains yes.

BMillman19 · 2018-10-15T21:03:33Z

packages/instant/src/components/order_details.tsx

+}) => {
+    const fontWeight = shouldEmphasize ? 700 : 400;
+    const usdFormatter = shouldConvertEthToUnitAmount ? format.ethBaseAmountInUsd : format.ethUnitAmountInUsd;
+    const ethFormatter = shouldConvertEthToUnitAmount ? format.ethBaseAmount : format.ethUnitAmount;


do we need to bind format here?

We don't use this in format, and even if we did we would be fine because we are using arrow functions. In general I think it's a bit awkward to use the function keyword in es6 since we have classes and arrow functions. In es5, all we had was function and that was for both classes and functions.

BMillman19 · 2018-10-15T21:07:11Z

packages/instant/src/components/buy_button.tsx

+        try {
+            const txnHash = await assetBuyer.executeBuyQuoteAsync(this.props.buyQuote, {
+                // HACK: There is a calculation issue in asset-buyer. ETH is refunded anyway so just over-estimate.
+                ethAmount: this.props.buyQuote.worstCaseQuoteInfo.totalEthAmount.mul(2),


i think this ethAmount should come from the buyQuoteInfo stored in redux so there is less chance we end up executing a buy thats different from what we are presenting in the order details

It's not technically possible in this implementation, but yes the API allows that... I feel like that's ok?

packages/instant/src/containers/selected_asset_buy_button.ts

BMillman19 · 2018-10-15T21:16:08Z

packages/instant/src/util/asset_buyer.ts

+const provider = getProvider();
+
+export const assetBuyer = AssetBuyer.getAssetBuyerForStandardRelayerAPIUrl(provider, sraApiUrl, {
+    expiryBufferSeconds: 300,


this should already be the default

BMillman19 · 2018-10-15T21:16:49Z

packages/instant/src/util/coinbase_api.ts

+import { BigNumber } from '@0xproject/utils';
+
+const baseEndpoint = 'https://api.coinbase.com/v2';
+export const coinbaseApi = {


is coinbase the best choice for this?

I'm not sure, jw what thought process you had when choosing the coinbase API to supply the price. Some things to consider, support for other currencies for localization, rate limiting, downtime, etc.

I looked into a few options but did not do a crazy thorough comparison. Coinbase has 3 req / sec, 10k req / hour per IP, which seems reasonable. They also support other (fiat) currencies. Not much info available on downtime. Do you know of other APIs?

steveklebanoff · 2018-10-15T23:05:22Z

packages/instant/src/components/amount_input.tsx

@@ -10,10 +10,13 @@ export interface AmountInputProps {
    fontColor?: ColorOption;
    fontSize?: string;
    value?: BigNumber;
-    onChange?: (value?: BigNumber) => void;
+    onChange: (value?: BigNumber) => void;


just curious -- why move this from being optional to being required but having a default no-op? is this a pattern we want to follow for callbacks?

So yea this is a bit confusing. @types/react and ts > 3.0.0 added understanding of defaultProps. So the way you think about the Props interface is that it's the internal interface (in this case, callback is not optional). However, if you have a default value the compiler knows that you actually don't have to pass the prop in (ie. the external interface is a function of internal interface + defaultProps).

So in practice it's not required, has default no-ops, so internally you can assume it's not undefined.

And yea I like this pattern :P

steveklebanoff · 2018-10-15T23:12:43Z

packages/instant/src/redux/actions.ts

+    UPDATE_LATEST_BUY_QUOTE = 'UPDATE_LATEST_BUY_QUOTE',
+}
+
+export const actions = {


typed actions! 🎉 🙌 💯

steveklebanoff · 2018-10-15T23:17:30Z

packages/instant/src/containers/latest_buy_quote_order_details.ts

+
+const mapStateToProps = (state: State, _ownProps: LatestBuyQuoteOrderDetailsProps): ConnectedState => ({
+    // use the worst case quote info
+    buyQuoteInfo: _.get(state, 'latestBuyQuote.worstCaseQuoteInfo'),


minor: I would prefer to grab the worstCaseInfo without using _.get to keep the typesafety (i.e. ensuring worstCaseQuoteInfo is actually a property of latestBuyQuote

I added a package https://github.com/rimeto/ts-optchain. The API is a bit ugly but it gives us the best of both worlds. Open to discussing, but didn't take long to include.

steveklebanoff · 2018-10-15T23:52:09Z

packages/instant/src/constants.ts

@@ -0,0 +1,6 @@
+import { BigNumber } from '@0xproject/utils';
+export const BIG_NUMBER_ZERO = new BigNumber(0);
+export const sraApiUrl = 'https://api.radarrelay.com/0x/v2/';


will we make this more configurable in a separate PR?

…into feature/instant/move-features-over-from-zrx-buyer

BMillman19 · 2018-10-15T21:22:00Z

packages/instant/src/util/format.ts

+import { ethDecimals } from '../constants';
+
+export const format = {
+    ethBaseAmount: (ethBaseAmount?: BigNumber, decimalPlaces: number = 4, defaultText: string = '0 ETH'): string => {


might be good to add unit tests here

BMillman19 · 2018-10-16T17:54:31Z

packages/instant/src/util/format.ts

+        if (_.isUndefined(ethUnitAmount) || _.isUndefined(ethUsdPrice)) {
+            return defaultText;
+        }
+        return `$${ethUnitAmount.mul(ethUsdPrice).round(decimalPlaces)}`;


sometimes this results in strings with 1 decimal place like: "$3.7"

…into feature/instant/move-features-over-from-zrx-buyer

coveralls · 2018-10-16T23:47:25Z

Coverage decreased (-0.009%) to 84.679% when pulling 32beeae on feature/instant/move-features-over-from-zrx-buyer into 72f5719 on development.

feat: Move over features from zrx-buyer

19f6190

fragosti added WIP @0x/instant labels Oct 11, 2018

fragosti requested review from steveklebanoff and BMillman19 October 11, 2018 01:37

fragosti commented Oct 11, 2018

View reviewed changes

fragosti added 9 commits October 11, 2018 15:05

feat: adjust amount input width

63652df

feat: debounce the fetching of new quotes

0edd9b3

fix: export BuyQuoteInfo from asset-buyer

1c92ae0

feat: populate order details with information from worst buy quote

03b235b

feat: have coinbase API return BigNumber for eth-usd price endpoint

09c5ae4

feat: make redux actions type-sage

f3391e1

feat: Add AssetData type as union of ERC721AssetData and ERC20AssetData

025614a

feat: use new AssetData type from types package

ccf021b

feat: model asset meta data and add dynamic assetData state

f395414

fragosti changed the title ~~[WIP][instant] Move over and clean up features from zrx-buyer~~ [instant] Move over and clean up features from zrx-buyer Oct 12, 2018

fragosti added @0x/types @0x/order-utils and removed WIP labels Oct 12, 2018

fragosti changed the title ~~[instant] Move over and clean up features from zrx-buyer~~ [instant][types][order-utils] Move over and clean up features from zrx-buyer Oct 12, 2018

fragosti changed the title ~~[instant][types][order-utils] Move over and clean up features from zrx-buyer~~ [instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer Oct 12, 2018

fragosti added the @0x/asset-buyer label Oct 12, 2018

fragosti added 2 commits October 15, 2018 14:11

Merge branch 'development' of https://github.com/0xProject/0x-monorepo …

e7130af

…into feature/instant/move-features-over-from-zrx-buyer

feat: add changelog entries for changed packages

43f8f2a

BMillman19 suggested changes Oct 15, 2018

View reviewed changes

steveklebanoff reviewed Oct 15, 2018

View reviewed changes

Add tnxHash to buy button callbacks

fcf3451

fragosti added 6 commits October 15, 2018 17:06

Put boundNoop in a util file

ac3bfdf

Rename OrderDetailsRow to EthAmountRow

fa18db8

Hide USD price when ETH-USD price is not available

18667d7

Add ts-optchain and use it instead of lodash get

f2e5fd8

Remove expiry buffer seconds option from AssetBuyer init

875f621

Merge branch 'development' of https://github.com/0xProject/0x-monorepo …

d268e19

…into feature/instant/move-features-over-from-zrx-buyer

BMillman19 approved these changes Oct 16, 2018

View reviewed changes

BMillman19 reviewed Oct 16, 2018

View reviewed changes

Merge branch 'development' of https://github.com/0xProject/0x-monorepo …

dbf5be6

…into feature/instant/move-features-over-from-zrx-buyer

fragosti force-pushed the feature/instant/move-features-over-from-zrx-buyer branch from a377e21 to dbf5be6 Compare October 16, 2018 21:54

fragosti added 2 commits October 16, 2018 16:13

Add tests for format and use toFixed instead of round for usd

2610868

Run tests on circle CI

c328616

fragosti added 4 commits October 16, 2018 16:58

Remove order-utils from dependencies

6a89935

chore: temporarily increase the bundle size for instant

d2adbc3

feat: export AssetData from utils

009b5b5

feat: export AssetData from order-utils

8ba6534

steveklebanoff mentioned this pull request Oct 17, 2018

[instant] Asset buyer errors #1142

Merged

8 tasks

fragosti added 2 commits October 16, 2018 17:48

fix: dont use enum string as type as typedoc gets confused

eda0b3e

Bump max bundle size for instant

32beeae

fragosti merged commit 336e456 into development Oct 17, 2018

fragosti deleted the feature/instant/move-features-over-from-zrx-buyer branch March 13, 2019 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131

[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131

fragosti commented Oct 11, 2018 •

edited

Loading

fragosti Oct 11, 2018

fragosti Oct 11, 2018

fragosti Oct 11, 2018

fragosti Oct 11, 2018

fragosti Oct 11, 2018

fragosti Oct 12, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

BMillman19 Oct 15, 2018

BMillman19 Oct 15, 2018

BMillman19 Oct 15, 2018

fragosti Oct 16, 2018

steveklebanoff Oct 15, 2018

fragosti Oct 16, 2018

steveklebanoff Oct 15, 2018

steveklebanoff Oct 15, 2018

fragosti Oct 16, 2018

steveklebanoff Oct 15, 2018

BMillman19 Oct 15, 2018

BMillman19 Oct 16, 2018

coveralls commented Oct 16, 2018 •

edited

Loading

[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131

[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131

Conversation

fragosti commented Oct 11, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 16, 2018 • edited Loading

fragosti commented Oct 11, 2018 •

edited

Loading

coveralls commented Oct 16, 2018 •

edited

Loading