-
Notifications
You must be signed in to change notification settings - Fork 465
[instant][types][order-utils][asset-buyer] Move over and clean up features from zrx-buyer #1131
Changes from 10 commits
19f6190
63652df
0edd9b3
1c92ae0
03b235b
09c5ae4
f3391e1
025614a
ccf021b
f395414
e7130af
43f8f2a
fcf3451
ac3bfdf
fa18db8
18667d7
f2e5fd8
875f621
d268e19
dbf5be6
2610868
c328616
6a89935
d2adbc3
009b5b5
8ba6534
eda0b3e
32beeae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,13 @@ export interface AmountInputProps { | |
fontColor?: ColorOption; | ||
fontSize?: string; | ||
value?: BigNumber; | ||
onChange?: (value?: BigNumber) => void; | ||
onChange: (value?: BigNumber) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So yea this is a bit confusing. @types/react and ts > 3.0.0 added understanding of 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 |
||
} | ||
|
||
export class AmountInput extends React.Component<AmountInputProps> { | ||
public static defaultProps = { | ||
onChange: _.noop.bind(_), | ||
}; | ||
public render(): React.ReactNode { | ||
const { fontColor, fontSize, value } = this.props; | ||
return ( | ||
|
@@ -24,7 +27,7 @@ export class AmountInput extends React.Component<AmountInputProps> { | |
onChange={this._handleChange} | ||
value={!_.isUndefined(value) ? value.toString() : ''} | ||
placeholder="0.00" | ||
width="2em" | ||
width="2.2em" | ||
/> | ||
</Container> | ||
); | ||
|
@@ -40,8 +43,6 @@ export class AmountInput extends React.Component<AmountInputProps> { | |
return; | ||
} | ||
} | ||
if (!_.isUndefined(this.props.onChange)) { | ||
this.props.onChange(bigNumberValue); | ||
} | ||
this.props.onChange(bigNumberValue); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { AssetProxyId } from '@0xproject/types'; | ||
import { BigNumber } from '@0xproject/utils'; | ||
import * as _ from 'lodash'; | ||
import * as React from 'react'; | ||
|
||
import { assetMetaData } from '../data/asset_meta_data'; | ||
import { ColorOption } from '../style/theme'; | ||
|
||
import { AmountInput, AmountInputProps } from './amount_input'; | ||
import { Container, Text } from './ui'; | ||
|
||
export interface AssetAmountInputProps extends AmountInputProps { | ||
assetData?: string; | ||
onChange: (value?: BigNumber, assetData?: string) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we ever going to have a case where we have a value but not an assetData? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice no, but those two things can be |
||
} | ||
|
||
export class AssetAmountInput extends React.Component<AssetAmountInputProps> { | ||
public static defaultProps = { | ||
onChange: _.noop.bind(_), | ||
}; | ||
public render(): React.ReactNode { | ||
const { assetData, onChange, ...rest } = this.props; | ||
return ( | ||
<Container> | ||
<AmountInput {...rest} onChange={this._handleChange} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do in a future PR (added to my list) |
||
<Container display="inline-block" marginLeft="10px"> | ||
<Text fontSize={rest.fontSize} fontColor={ColorOption.white} textTransform="uppercase"> | ||
{this._getLabel()} | ||
</Text> | ||
</Container> | ||
</Container> | ||
); | ||
} | ||
private readonly _getLabel = (): string => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this can have a clearer name like |
||
const unknownLabel = '???'; | ||
if (_.isUndefined(this.props.assetData)) { | ||
return unknownLabel; | ||
} | ||
const metaData = assetMetaData[this.props.assetData]; | ||
if (_.isUndefined(metaData)) { | ||
return unknownLabel; | ||
} | ||
if (metaData.assetProxyId === AssetProxyId.ERC20) { | ||
return metaData.symbol; | ||
} | ||
return unknownLabel; | ||
}; | ||
private readonly _handleChange = (value?: BigNumber): void => { | ||
this.props.onChange(value, this.props.assetData); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,56 @@ | ||
import { BuyQuote } from '@0xproject/asset-buyer'; | ||
import * as _ from 'lodash'; | ||
import * as React from 'react'; | ||
|
||
import { ColorOption } from '../style/theme'; | ||
import { assetBuyer } from '../util/asset_buyer'; | ||
import { web3Wrapper } from '../util/web3_wrapper'; | ||
|
||
import { Button, Container, Text } from './ui'; | ||
|
||
export interface BuyButtonProps {} | ||
export interface BuyButtonProps { | ||
buyQuote?: BuyQuote; | ||
onClick: (buyQuote: BuyQuote) => void; | ||
onBuySuccess: (buyQuote: BuyQuote) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we might want to return the txHash as a param for the callback as well |
||
onBuyFailure: (buyQuote: BuyQuote) => void; | ||
text: string; | ||
} | ||
|
||
export const BuyButton: React.StatelessComponent<BuyButtonProps> = props => ( | ||
<Container padding="20px" width="100%"> | ||
<Button width="100%"> | ||
<Text fontColor={ColorOption.white} fontWeight={600} fontSize="20px"> | ||
Buy | ||
</Text> | ||
</Button> | ||
</Container> | ||
); | ||
const boundNoop = _.noop.bind(_); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linter complains yes. |
||
|
||
BuyButton.displayName = 'BuyButton'; | ||
export class BuyButton extends React.Component<BuyButtonProps> { | ||
public static defaultProps = { | ||
onClick: boundNoop, | ||
onBuySuccess: boundNoop, | ||
onBuyFailure: boundNoop, | ||
}; | ||
public render(): React.ReactNode { | ||
const shouldDisableButton = _.isUndefined(this.props.buyQuote); | ||
return ( | ||
<Container padding="20px" width="100%"> | ||
<Button width="100%" onClick={this._handleClick} isDisabled={shouldDisableButton}> | ||
<Text fontColor={ColorOption.white} fontWeight={600} fontSize="20px"> | ||
{this.props.text} | ||
</Text> | ||
</Button> | ||
</Container> | ||
); | ||
} | ||
private readonly _handleClick = async () => { | ||
// The button is disabled when there is no buy quote anyway. | ||
if (_.isUndefined(this.props.buyQuote)) { | ||
return; | ||
} | ||
this.props.onClick(this.props.buyQuote); | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets not commit this, the fix should be up from the protocol side early this week |
||
ethAmount: this.props.buyQuote.worstCaseQuoteInfo.totalEthAmount.mul(2), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not technically possible in this implementation, but yes the API allows that... I feel like that's ok? |
||
}); | ||
await web3Wrapper.awaitTransactionSuccessAsync(txnHash); | ||
} catch { | ||
this.props.onBuyFailure(this.props.buyQuote); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to be if the tx fails? I'm not 100% sure if awaitTransactionSuccessAsync will throw. We might want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It throws. Sweet! |
||
} | ||
this.props.onBuySuccess(this.props.buyQuote); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,32 @@ | ||
import { BigNumber } from '@0xproject/utils'; | ||
import * as _ from 'lodash'; | ||
import * as React from 'react'; | ||
|
||
import { SelectedAssetAmountInput } from '../containers/selected_asset_amount_input'; | ||
import { ColorOption } from '../style/theme'; | ||
import { format } from '../util/format'; | ||
|
||
import { Container, Flex, Text } from './ui'; | ||
|
||
export interface InstantHeadingProps {} | ||
export interface InstantHeadingProps { | ||
selectedAssetAmount?: BigNumber; | ||
totalEthBaseAmount?: BigNumber; | ||
ethUsdPrice?: BigNumber; | ||
} | ||
|
||
const displaytotalEthBaseAmount = ({ selectedAssetAmount, totalEthBaseAmount }: InstantHeadingProps): string => { | ||
if (_.isUndefined(selectedAssetAmount)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this logic just live in format? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see format as just formatting ethereum amounts into strings (including ETH dollar amounts). This case is handling the input not having any value, which seems to specific. |
||
return '0 ETH'; | ||
} | ||
return format.ethBaseAmount(totalEthBaseAmount, 4, '...loading'); | ||
}; | ||
|
||
const displayUsdAmount = ({ totalEthBaseAmount, selectedAssetAmount, ethUsdPrice }: InstantHeadingProps): string => { | ||
if (_.isUndefined(selectedAssetAmount)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
return '$0.00'; | ||
} | ||
return format.ethBaseAmountInUsd(totalEthBaseAmount, ethUsdPrice, 2, '...loading'); | ||
}; | ||
|
||
export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = props => ( | ||
<Container backgroundColor={ColorOption.primaryColor} padding="20px" width="100%" borderRadius="3px 3px 0px 0px"> | ||
|
@@ -22,22 +43,15 @@ export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = pro | |
</Text> | ||
</Container> | ||
<Flex direction="row" justify="space-between"> | ||
<Container> | ||
<SelectedAssetAmountInput fontSize="45px" /> | ||
<Container display="inline-block" marginLeft="10px"> | ||
<Text fontSize="45px" fontColor={ColorOption.white} textTransform="uppercase"> | ||
rep | ||
</Text> | ||
</Container> | ||
</Container> | ||
<SelectedAssetAmountInput fontSize="45px" /> | ||
<Flex direction="column" justify="space-between"> | ||
<Container marginBottom="5px"> | ||
<Text fontSize="16px" fontColor={ColorOption.white} fontWeight={500}> | ||
0 ETH | ||
{displaytotalEthBaseAmount(props)} | ||
</Text> | ||
</Container> | ||
<Text fontSize="16px" fontColor={ColorOption.white} opacity={0.7}> | ||
$0.00 | ||
{displayUsdAmount(props)} | ||
</Text> | ||
</Flex> | ||
</Flex> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,53 +1,85 @@ | ||
import { BuyQuoteInfo } from '@0xproject/asset-buyer'; | ||
import { BigNumber } from '@0xproject/utils'; | ||
import * as _ from 'lodash'; | ||
import * as React from 'react'; | ||
|
||
import { ColorOption } from '../style/theme'; | ||
import { format } from '../util/format'; | ||
|
||
import { Container, Flex, Text } from './ui'; | ||
|
||
export interface OrderDetailsProps {} | ||
|
||
export const OrderDetails: React.StatelessComponent<OrderDetailsProps> = props => ( | ||
<Container padding="20px" width="100%"> | ||
<Container marginBottom="10px"> | ||
<Text | ||
letterSpacing="1px" | ||
fontColor={ColorOption.primaryColor} | ||
fontWeight={600} | ||
textTransform="uppercase" | ||
fontSize="14px" | ||
> | ||
Order Details | ||
</Text> | ||
</Container> | ||
<OrderDetailsRow name="Token Price" primaryValue=".013 ETH" secondaryValue="$24.32" /> | ||
<OrderDetailsRow name="Fee" primaryValue=".005 ETH" secondaryValue="$1.04" /> | ||
<OrderDetailsRow name="Total Cost" primaryValue="1.66 ETH" secondaryValue="$589.56" shouldEmphasize={true} /> | ||
</Container> | ||
); | ||
export interface OrderDetailsProps { | ||
buyQuoteInfo?: BuyQuoteInfo; | ||
ethUsdPrice?: BigNumber; | ||
} | ||
|
||
OrderDetails.displayName = 'OrderDetails'; | ||
export class OrderDetails extends React.Component<OrderDetailsProps> { | ||
public render(): React.ReactNode { | ||
const { buyQuoteInfo, ethUsdPrice } = this.props; | ||
const ethAssetPrice = _.get(buyQuoteInfo, 'ethPerAssetPrice'); | ||
const ethTokenFee = _.get(buyQuoteInfo, 'feeEthAmount'); | ||
const totalEthAmount = _.get(buyQuoteInfo, 'totalEthAmount'); | ||
return ( | ||
<Container padding="20px" width="100%"> | ||
<Container marginBottom="10px"> | ||
<Text | ||
letterSpacing="1px" | ||
fontColor={ColorOption.primaryColor} | ||
fontWeight={600} | ||
textTransform="uppercase" | ||
fontSize="14px" | ||
> | ||
Order Details | ||
</Text> | ||
</Container> | ||
<OrderDetailsRow | ||
name="Token Price" | ||
ethAmount={ethAssetPrice} | ||
ethUsdPrice={ethUsdPrice} | ||
shouldConvertEthToUnitAmount={false} | ||
/> | ||
<OrderDetailsRow name="Fee" ethAmount={ethTokenFee} ethUsdPrice={ethUsdPrice} /> | ||
<OrderDetailsRow | ||
name="Total Cost" | ||
ethAmount={totalEthAmount} | ||
ethUsdPrice={ethUsdPrice} | ||
shouldEmphasize={true} | ||
/> | ||
</Container> | ||
); | ||
} | ||
} | ||
|
||
export interface OrderDetailsRowProps { | ||
name: string; | ||
primaryValue: string; | ||
secondaryValue: string; | ||
ethAmount?: BigNumber; | ||
shouldConvertEthToUnitAmount?: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this mean? comments for this props definition would be good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just renamed everything hopefully this is clearer. |
||
ethUsdPrice?: BigNumber; | ||
shouldEmphasize?: boolean; | ||
} | ||
|
||
export const OrderDetailsRow: React.StatelessComponent<OrderDetailsRowProps> = props => { | ||
const fontWeight = props.shouldEmphasize ? 700 : 400; | ||
export const OrderDetailsRow: React.StatelessComponent<OrderDetailsRowProps> = ({ | ||
name, | ||
ethAmount, | ||
shouldConvertEthToUnitAmount, | ||
ethUsdPrice, | ||
shouldEmphasize, | ||
}) => { | ||
const fontWeight = shouldEmphasize ? 700 : 400; | ||
const usdFormatter = shouldConvertEthToUnitAmount ? format.ethBaseAmountInUsd : format.ethUnitAmountInUsd; | ||
const ethFormatter = shouldConvertEthToUnitAmount ? format.ethBaseAmount : format.ethUnitAmount; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to bind format here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't use |
||
return ( | ||
<Container padding="10px 0px" borderTop="1px dashed" borderColor={ColorOption.feintGrey}> | ||
<Flex justify="space-between"> | ||
<Text fontWeight={fontWeight} fontColor={ColorOption.grey}> | ||
{props.name} | ||
{name} | ||
</Text> | ||
<Container> | ||
<Container marginRight="3px" display="inline-block"> | ||
<Text fontColor={ColorOption.lightGrey}>({props.secondaryValue}) </Text> | ||
<Text fontColor={ColorOption.lightGrey}>({usdFormatter(ethAmount, ethUsdPrice)})</Text> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are missing the ETH/USD conversion we probably shouldn't render the parentheses |
||
</Container> | ||
<Text fontWeight={fontWeight} fontColor={ColorOption.grey}> | ||
{props.primaryValue} | ||
{ethFormatter(ethAmount)} | ||
</Text> | ||
</Container> | ||
</Flex> | ||
|
@@ -57,6 +89,7 @@ export const OrderDetailsRow: React.StatelessComponent<OrderDetailsRowProps> = p | |
|
||
OrderDetailsRow.defaultProps = { | ||
shouldEmphasize: false, | ||
shouldConvertEthToUnitAmount: true, | ||
}; | ||
|
||
OrderDetailsRow.displayName = 'OrderDetailsRow'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,21 @@ | ||
import * as React from 'react'; | ||
|
||
import { LatestBuyQuoteOrderDetails } from '../containers/latest_buy_quote_order_details'; | ||
import { SelectedAssetBuyButton } from '../containers/selected_asset_buy_button'; | ||
import { SelectedAssetInstantHeading } from '../containers/selected_asset_instant_heading'; | ||
|
||
import { ColorOption } from '../style/theme'; | ||
|
||
import { BuyButton } from './buy_button'; | ||
import { InstantHeading } from './instant_heading'; | ||
import { OrderDetails } from './order_details'; | ||
import { Container, Flex } from './ui'; | ||
|
||
export interface ZeroExInstantContainerProps {} | ||
|
||
export const ZeroExInstantContainer: React.StatelessComponent<ZeroExInstantContainerProps> = props => ( | ||
<Container hasBoxShadow={true} width="350px" backgroundColor={ColorOption.white} borderRadius="3px"> | ||
<Flex direction="column" justify="flex-start"> | ||
<InstantHeading /> | ||
<OrderDetails /> | ||
<BuyButton /> | ||
<SelectedAssetInstantHeading /> | ||
<LatestBuyQuoteOrderDetails /> | ||
<SelectedAssetBuyButton /> | ||
</Flex> | ||
</Container> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is value undefined if the input is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! This is what made most sense to me.