[instant] Asset buyer errors #1142

steveklebanoff · 2018-10-17T00:27:26Z

Description

Shows error messages for AssetBuyer getBuyQuoteAsync errors

Also:

adds quoteState to keep track of the status of the current quote
shows "--" if we get an error when fetching in top right
renamed selectedAssetBuyState to buyOrderState
handling of additional AssetBuyer errors
tests for bestNameForAsset
polish

Testing instructions

Types of changes

New feature

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.

steveklebanoff · 2018-10-17T00:33:19Z

packages/instant/src/util/error_flasher.ts

@@ -0,0 +1,27 @@
+import { Dispatch } from 'redux';


Wasn't sure the right place for this file, open to suggestions

steveklebanoff · 2018-10-17T00:36:28Z

packages/instant/src/containers/latest_error.tsx

+
+export interface LatestErrorComponentProps {
+    assetData?: string;
+    latestError?: any;


Per convo w/ @fragosti , we explored two options:

When error is "cleared" (after timeout), set currentError to be undefined. This would have been nice because it's the most straightforward representation of the current state, but animating the un-mounting of the component would either be pretty hairy or involve introducing a new dependency.

Keep the error around in latestError but set latestErrorDismissed to true, and then render the sliding down error message. I don't love keeping the last error around but treating it as "dismissed", but it makes the animation logic simple.

After discussing, I went with option 2 for the drawbacks listed in option 1. Open to feedback though.

steveklebanoff · 2018-10-17T00:37:14Z

packages/instant/src/redux/store.ts

@@ -3,4 +3,5 @@ import { createStore, Store as ReduxStore } from 'redux';

 import { reducer, State } from './reducer';

-export const store: ReduxStore<State> = createStore(reducer);
+const reduxDevTools = (window as any).__REDUX_DEVTOOLS_EXTENSION__;


For using redux dev tools in chrome. I couldn't find a pattern for this in the website code, so went with this (window as any) hack

In the website its setup so that it's only being connected in development https://github.com/zalmoxisus/redux-devtools-extension, however I think this is fine?

I like using the package and having it be dev only, fixed in 3f0d7c70bd156edc4039d08737e74e99c0b3a04e

fragosti · 2018-10-17T00:54:25Z

packages/instant/src/redux/store.ts

@@ -3,4 +3,5 @@ import { createStore, Store as ReduxStore } from 'redux';

 import { reducer, State } from './reducer';

-export const store: ReduxStore<State> = createStore(reducer);
+const reduxDevTools = (window as any).__REDUX_DEVTOOLS_EXTENSION__;


In the website its setup so that it's only being connected in development https://github.com/zalmoxisus/redux-devtools-extension, however I think this is fine?

fragosti · 2018-10-17T00:55:39Z

packages/instant/src/redux/reducer.ts

    selectedAssetData?: string;
    selectedAssetAmount?: BigNumber;
    selectedAssetBuyState: AsyncProcessState;
    ethUsdPrice?: BigNumber;
    latestBuyQuote?: BuyQuote;
 }
+interface StateWithError extends BaseState {
+    latestError: any;
+    latestErrorDismissed: boolean;


needs to follow boolean naming convention (ie. "was", "is", "should" etc...)

packages/instant/src/util/asset_data.ts

fragosti · 2018-10-17T00:58:53Z

packages/instant/src/util/asset_data.ts

+
+// TODO: tests for this
+export const bestNameForAsset = (assetData: string | undefined, defaultString: string) => {
+    if (assetData === undefined) {


we use _.isUndefined pretty consistently for this.

ah I noticed that, but I try to avoid importing a library function if it's simple, easy and readable to perfrom the same thing without it, a la https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore

Yea I kind of agree? Don't feel too strongly. However, as mentioned previously, I am a big fan of consistency and reducing decisions. It's nice to not have to think about "how am I going to check for undefined". And it's also nice for the entire codebase to do it the same way. Idk, I think since we already have lodash in the bundle it's worth doing that, but don't feel strongly about it.

I think since we already have lodash in the bundle it's worth doing that

If we are able to get our bundle optimized, the bundle should only include the lodash functions that we use, and it'd be nice to be pulling in one less function if we consistently used this approach for checking for undefined

That being said, I hear you RE: consistency and reducing decisions. For the sake of those two points, I'm down to use the lodash function for now. We can revisit our approach to lodash in general as a team later, and perhaps establish a new consistent pattern moving forward at that point.

fragosti · 2018-10-17T01:06:32Z

packages/instant/src/components/zero_ex_instant_container.tsx

@@ -12,6 +13,9 @@ export interface ZeroExInstantContainerProps {}

 export const ZeroExInstantContainer: React.StatelessComponent<ZeroExInstantContainerProps> = props => (
    <Container width="350px">
+        <Container zIndex={1} position="relative">
+            <LatestError />
+        </Container>


fragosti · 2018-10-17T01:20:32Z

packages/instant/src/containers/latest_error.tsx

+    const slidingDirection = props.latestErrorDismissed ? 'down' : 'up';
+    const { icon, message } = errorDescription(props.latestError, props.assetData);
+    return <SlidingError direction={slidingDirection} icon={icon} message={message} />;
+};


A convention I'm trying to follow is to not have any Component code in these container files, they simply connect a pure presentational component to the redux store and do any state -> prop mapping. Seems like you don't need this component to me?

The sliding direction logjc can just be done in mapStateToProps.

icon is a pure function of state.

message is a pure function of state.

The only thing is not rendering when latestError is not defined, but do we need that? The error hides behind container.

I like the idea of getting rid of this component, but I think we need the custom "not render when latestError is undefined" logic. Some thoughts:

If there is no latestError, we wouldn't have anything to send in to SlidingError's icon and message props. I like keeping these props as required instead of optional.

Even if we rendered SlidingError without icon and message props, in the current implementation, rendering it in the "Slide Down" phase would have it pop up at the top and slide down, which is not what we want. We could modify SlidingError to return a blank div if icon and message are empty, but that doesn't feel quite right to me.

I may be missing something though, let me know if there is an alternative approach in which we could get rid of this component.

If you agree we need to keep the component, I understand the convention and pattern of separating presentational and container components, but in this case it seems overboard IMHO: this component is extremely simple and it's highly unlikely it'd be used anywhere else.

You could change it so that latestError sticks around and you simply toggle it up and down in the flasher / redux. This kind of seems better to me because then it becomes impossible to make the error just disappear?

Per slack convo, decided to leave as-is and address later if needed

fragosti · 2018-10-17T01:22:57Z

packages/instant/src/redux/reducer.ts

    selectedAssetData?: string;
    selectedAssetAmount?: BigNumber;
    selectedAssetBuyState: AsyncProcessState;
    ethUsdPrice?: BigNumber;
    latestBuyQuote?: BuyQuote;
 }
+interface StateWithError extends BaseState {
+    latestError: any;
+    latestErrorDismissed: boolean;


what if we call this latestErrorState and it can be either Present or Hidden or something. And lets not allow it to be undefined.

I like this -- feels more clear than undefined or a boolean 💯. Will update!

fragosti · 2018-10-17T01:25:27Z

packages/instant/src/util/error_description.ts

+        icon: '😢',
+        message: bestMessage || 'Something went wrong...',
+    };
+};


I would perhaps create one util/error.ts file that has one error or errorUtil object export and also provides the flashNewError utility?

I'll try this and see how it feels once implemented. ErrorFlasher is kinda big though, not sure if I love it being grouped together with other things

Done in d45ffb33513057789a2e1cf03a9f01ee05846e17

fragosti · 2018-10-17T01:26:42Z

packages/instant/src/util/asset_data.ts

+import { assetMetaData } from '../data/asset_meta_data';
+
+// TODO: tests for this
+export const bestNameForAsset = (assetData: string | undefined, defaultString: string) => {


I like the way this is structured but I'd like to follow our somewhat soft convention of exporting one thing from file.
So the api would be assetDataUtils.bestName()

I wonder if this convention could be bad for optimizing builds. Not sure. Cool w/ keeping convention for now though

coveralls · 2018-10-17T02:25:19Z

Coverage increased (+0.009%) to 84.65% when pulling 6ea386a on feature/instant/asset-buyer-errors into 5ec4b27 on development.

coveralls · 2018-10-17T02:26:30Z

Coverage remained the same at 84.688% when pulling 241846e on feature/instant/asset-buyer-errors into 8ba6534 on feature/instant/move-features-over-from-zrx-buyer.

BMillman19 · 2018-10-17T07:09:15Z

packages/instant/src/redux/reducer.ts

+    latestError: undefined;
+    latestErrorDismissed: undefined;
+}
+export type State = StateWithError | StateWithoutError;


What if we had two types, BaseState that looks like above and ErrorState that looks like this:

interface ErrorState { latestError?: any; latestErrorDismissed?: boolean; }

the final type is:

type State = BaseState & ErrorState

BaseState could even be broken down further

I like this idea -- but I think with the change to Present | Hidden type which is always present might not be as important

fragosti

Looking good overall. Feel free to address issues how you see fit :)

fragosti · 2018-10-17T18:46:15Z

packages/instant/src/containers/latest_error.tsx

+    const slidingDirection = props.latestErrorDismissed ? 'down' : 'up';
+    const { icon, message } = errorDescription(props.latestError, props.assetData);
+    return <SlidingError direction={slidingDirection} icon={icon} message={message} />;
+};


You could change it so that latestError sticks around and you simply toggle it up and down in the flasher / redux. This kind of seems better to me because then it becomes impossible to make the error just disappear?

…index to be set on container

fragosti · 2018-10-18T01:26:06Z

packages/instant/src/components/instant_heading.tsx

@@ -12,20 +13,45 @@ export interface InstantHeadingProps {
    selectedAssetAmount?: BigNumber;
    totalEthBaseAmount?: BigNumber;
    ethUsdPrice?: BigNumber;
+    quoteState: AsyncProcessState;


I still think this should be called quoteRequestState. quote is data, and it doesn't make sense to me for it to have state.

fragosti · 2018-10-18T16:01:10Z

packages/instant/src/components/instant_heading.tsx

@@ -47,11 +73,11 @@ export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = pro
            <Flex direction="column" justify="space-between">
                <Container marginBottom="5px">
                    <Text fontSize="16px" fontColor={ColorOption.white} fontWeight={500}>
-                        {displaytotalEthBaseAmount(props)}
+                        {loadingOrAmount(props.quoteState, displaytotalEthBaseAmount(props))}


Feels kind of weird to have two nested Text components. Can't displayTotalEthUsdAmount take care of all of this?

Good call -- yes, will fix

fragosti · 2018-10-18T16:05:44Z

packages/instant/src/containers/selected_asset_amount_input.ts

+
+        if (!_.isUndefined(value) && !_.isUndefined(assetData)) {
+            // even if it's debounced, give them the illusion it's loading
+            dispatch(actions.updateBuyQuoteStatePending());


ah good idea

fragosti · 2018-10-18T16:06:54Z

packages/instant/src/redux/actions.ts

 }

 export const actions = {
    updateEthUsdPrice: (price?: BigNumber) => createAction(ActionTypes.UPDATE_ETH_USD_PRICE, price),
    updateSelectedAssetAmount: (amount?: BigNumber) => createAction(ActionTypes.UPDATE_SELECTED_ASSET_AMOUNT, amount),
-    updateSelectedAssetBuyState: (buyState: AsyncProcessState) =>
+    updatebuyOrderState: (buyState: AsyncProcessState) =>


nit: this is not camelCase

fragosti · 2018-10-18T16:08:17Z

packages/instant/src/redux/actions.ts

@@ -25,12 +25,22 @@ export enum ActionTypes {
    UPDATE_SELECTED_ASSET_AMOUNT = 'UPDATE_SELECTED_ASSET_AMOUNT',
    UPDATE_SELECTED_ASSET_BUY_STATE = 'UPDATE_SELECTED_ASSET_BUY_STATE',
    UPDATE_LATEST_BUY_QUOTE = 'UPDATE_LATEST_BUY_QUOTE',
+    UPDATE_BUY_QUOTE_STATE_PENDING = 'UPDATE_BUY_QUOTE_STATE_PENDING',
+    UPDATE_BUY_QUOTE_STATE_FAILURE = 'UPDATE_BUY_QUOTE_STATE_FAILURE',


SET_BUY_QUOTE_STATE_PENDING & SET_BUY_QUOTE_STATE_FAILURE ? Update means it takes an argument.

Ah, wasn't hip to this convention. Will change.

fragosti · 2018-10-18T16:09:03Z

packages/instant/src/redux/reducer.ts

@@ -7,21 +7,31 @@ import { AsyncProcessState } from '../types';

 import { Action, ActionTypes } from './actions';

+export enum LatestErrorDisplay {


This should go in types.ts and can probably be more general.

fragosti · 2018-10-18T16:10:36Z

packages/instant/src/util/error.ts

+
+export const errorUtil = {
+    errorFlasher: new ErrorFlasher(),
+    errorDescription: (error?: any, assetData?: string): { icon: string; message: string } => {


Thanks for doing this!

steveklebanoff added WIP @0x/instant labels Oct 17, 2018

steveklebanoff requested a review from fragosti October 17, 2018 00:27

steveklebanoff requested a review from BMillman19 as a code owner October 17, 2018 00:27

steveklebanoff commented Oct 17, 2018

View reviewed changes

fragosti reviewed Oct 17, 2018

View reviewed changes

BMillman19 reviewed Oct 17, 2018

View reviewed changes

fragosti approved these changes Oct 17, 2018

View reviewed changes

steveklebanoff added 14 commits October 17, 2018 14:44

feat(instant): add sliding error

18c9907

move z-index setting to zero instant container, and add ability for z…

f36352b

…index to be set on container

feat(instant): Handle AssetBuyer errors

db77cd1

linting, removing unused imports

1d07949

get rid of unused marginLeft and marginRight props

dfc5d7d

use redux dev tools from package

d46b288

latestErrorDismissed -> latestErrorDisplay enum

187bbc7

export assetDataUtil big obj

32fa1bc

use lodash isUndefined per PR comment

d052342

creating big error util file per francescos comment in PR

155858d

bigger emoji

2b495a7

handle other errors

ae4f1a0

asset data util tests

f2f7598

error test

7b43cd1

steveklebanoff force-pushed the feature/instant/asset-buyer-errors branch from 2d7eb9a to 7b43cd1 Compare October 17, 2018 21:45

steveklebanoff requested a review from fabioberger as a code owner October 17, 2018 21:45

steveklebanoff changed the base branch from feature/instant/move-features-over-from-zrx-buyer to development October 17, 2018 21:45

add concept of quoteState

6cf8d57

ReactNode as default for formatters, and show bold dash

01b98c3

steveklebanoff changed the title ~~[WIP][instant] Asset buyer errors~~ [instant] Asset buyer errors Oct 17, 2018

steveklebanoff added 2 commits October 17, 2018 16:49

fix type

1d38b75

selectedAssetBuyState -> buyOrderState

6ea386a

steveklebanoff removed the WIP label Oct 18, 2018

fabioberger approved these changes Oct 18, 2018

View reviewed changes

fabioberger merged commit d0df074 into development Oct 18, 2018

fabioberger deleted the feature/instant/asset-buyer-errors branch October 18, 2018 10:12

fragosti reviewed Oct 18, 2018

View reviewed changes

steveklebanoff mentioned this pull request Oct 18, 2018

[instant] Order details loading state #1161

Merged

5 tasks

		@@ -7,21 +7,31 @@ import { AsyncProcessState } from '../types';

		import { Action, ActionTypes } from './actions';

		export enum LatestErrorDisplay {

[instant] Asset buyer errors #1142

[instant] Asset buyer errors #1142

Conversation

steveklebanoff commented Oct 17, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

steveklebanoff Oct 17, 2018 • edited Loading

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 17, 2018 • edited Loading

coveralls commented Oct 17, 2018

BMillman19 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fragosti left a 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

steveklebanoff commented Oct 17, 2018 •

edited

Loading

steveklebanoff Oct 17, 2018 •

edited

Loading

coveralls commented Oct 17, 2018 •

edited

Loading

BMillman19 Oct 17, 2018 •

edited

Loading