[asset-buyer] Create new AssetBuyer class #1037

BMillman19 · 2018-08-28T21:25:52Z

Description

AssetBuyer class

Testing instructions

TBD

Types of changes

New feature (non-breaking change which adds functionality)

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.

coveralls · 2018-08-28T21:45:22Z

Coverage increased (+0.005%) to 85.179% when pulling d8d1c98 on feature/forwarder-helper/sra-and-rpc into 8bce407 on development.

BMillman19 · 2018-08-28T22:26:43Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+     */
+    async getForwarderHelperForMakerAssetDataAsync(
+        makerAssetData: string,
+        takerAddress: string,


remove this since we do not actually need any on chain info for the taker

BMillman19 · 2018-08-28T22:28:23Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+        const [makerAssetOrderbook, zrxOrderbook] = await Promise.all(
+            _.map(orderbookRequests, request => sraClient.getOrderbookAsync(request, requestOpts)),
+        );
+        // validate orders and find remaining fillable from on chain state or sra api


we potentially want to filter out orders where the taker is not null address (i.e. not an open order)

BMillman19 · 2018-08-28T22:29:57Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+        }
+        providerEngine.start();
+        // create contract wrappers given provider and networkId
+        const contractWrappers = new ContractWrappers(providerEngine, { networkId });


we might be able to extract out the contractWrappers creation into a different factory to make this function a bit shorter, we will also want to use the wrappers to validate orders that don't come from an SRA endpoint

BMillman19 · 2018-08-28T22:31:25Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+            { baseAssetData: zrxTokenAssetData, quoteAssetData: etherTokenAssetData },
+        ];
+        const requestOpts = { networkId };
+        // TODO: try catch these requests and throw a more domain specific error


STANDARD_RELAYER_API_ERROR

BMillman19 · 2018-08-28T22:32:01Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+            // if we do have an rpc url, get on-chain orders and traders info via the OrderValidatorWrapper
+            const ordersFromSra = _.map(makerAssetOrderbook.asks.records, apiOrder => apiOrder.order);
+            const feeOrdersFromSra = _.map(zrxOrderbook.asks.records, apiOrder => apiOrder.order);
+            // TODO: try catch these requests and throw a more domain specific error


ETHEREUM_NODE_RPC_ERROR

BMillman19 · 2018-08-28T22:33:01Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+            // TODO: try catch these requests and throw a more domain specific error
+            const [makerAssetOrdersAndTradersInfo, feeOrdersAndTradersInfo] = await Promise.all(
+                _.map([ordersFromSra, feeOrdersFromSra], ordersToBeValidated => {
+                    const takerAddresses = _.map(ordersToBeValidated, () => takerAddress);


change this to NULL_ADDRESS in conjunction w/ removing takerAddress from the function args

BMillman19 · 2018-08-28T22:34:25Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+): OrdersAndRemainingFillableMakerAssetAmounts {
+    const result = _.reduce(
+        apiOrders,
+        (acc, apiOrder, index) => {


index is not used here

BMillman19 · 2018-08-28T22:36:32Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+            if (orderUtils.isOrderExpired(order)) {
+                return acc;
+            }
+            // calculate remainingFillableMakerAssetAmount from api metadata


add comment about trying to grab remaining taker amount from SRA metadata, else, assume order is completely fillable

BMillman19 · 2018-08-28T22:37:16Z

packages/forwarder-helper/src/forwarder_helper_factory.ts

+            const { orders, remainingFillableMakerAssetAmounts } = acc;
+            // get corresponding on-chain state for the order
+            const { orderInfo, traderInfo } = ordersAndTradersInfo[index];
+            // if the order IS NOT fillable, do not add anything and continue iterating


"do not add anything to the accumulations"

BMillman19 · 2018-08-28T22:38:59Z

packages/forwarder-helper/src/utils/order_utils.ts

+    },
+    calculateRemainingMakerAssetAmount(order: SignedOrder, remainingTakerAssetAmount: BigNumber): BigNumber {
+        const result = remainingTakerAssetAmount.eq(0)
+            ? new BigNumber(0)


constants.ZERO_AMOUNT

BMillman19 · 2018-08-28T23:04:26Z

packages/forwarder-helper/src/forwarder_helper_factory.ts


 export const forwarderHelperFactory = {
    /**
-     * Given an array of orders and an array of feeOrders
+     * Given an array of orders and an array of feeOrders, get a ForwarderHelper


Add checks here for orders all having the same makerAsset and takerAsset

…rFactory

fragosti

Need to add StandardRelayerApiAssetBuyerManager

fragosti · 2018-09-17T16:28:13Z

packages/asset-buyer/src/asset_buyer.ts

+const SLIPPAGE_PERCENTAGE = 0.2; // 20% slippage protection, possibly move this into request interface
+const DEFAULT_ORDER_REFRESH_INTERVAL_MS = 10000; // 10 seconds
+const DEFAULT_FEE_PERCENTAGE = 0;
+const ETHER_TOKEN_DECIMALS = 18;


Move to constants file

fragosti · 2018-09-17T16:34:36Z

packages/asset-buyer/src/asset_buyer.ts

+     *                                  Defaults to 10000ms (10s).
+     * @return  An instance of AssetBuyer
+     */
+    constructor(


Do we want to have a factory method for providing your own orders but not your own fee orders (and passing in an sraUrl instead?)

fragosti · 2018-09-17T17:08:34Z

packages/asset-buyer/src/types.ts

+    feeOrders: SignedOrderWithRemainingFillableMakerAssetAmount[];
+    remainingFillableMakerAssetAmounts: BigNumber[];
+    remainingFillableFeeAmounts: BigNumber[];
+}


do we need remainingFillableMakerAssetAmounts and remainingFillableFeeAmounts now that we have SignedOrderWithRemainingFillableMakerAssetAmount ?

Seems like we should choose one format over the other.

fragosti · 2018-09-17T17:22:15Z

packages/asset-buyer/src/utils/order_fetcher_response_processor.ts

+    orders: SignedOrderWithRemainingFillableMakerAssetAmount[],
+): SignedOrderWithRemainingFillableMakerAssetAmount[] {
+    const result = _.filter(orders, order => {
+        return orderUtils.isOpenOrder(order) && orderUtils.isOrderExpired(order);


should probably be!orderUtils.isOrderExpired

…gNumber

fabioberger · 2018-09-20T09:10:57Z

packages/asset-buyer/README.md


-Provides convenience objects to help work with the Forwarder Contract
+Convenience package for buying assets


Can you please provide a more elaborate description, this is too vague.

fabioberger · 2018-09-20T09:14:23Z

packages/asset-buyer/src/asset_buyer.ts

+    private _lastRefreshTimeIfExists?: number;
+    private _currentOrdersAndFillableAmountsIfExists?: AssetBuyerOrdersAndFillableAmounts;
+    /**
+     * Instantiates a new AssetBuyer instance


Improve this comment and the subsequent method comment, they are identical but these methods are slightly different.

fabioberger · 2018-09-20T09:16:51Z

packages/asset-buyer/src/asset_buyer.ts

+        return assetBuyer;
+    }
+    /**
+     * Instantiates a new AssetBuyer instance


Ditto above.

fabioberger · 2018-09-20T09:19:03Z

packages/asset-buyer/src/asset_buyer.ts

+            orderRefreshIntervalMs,
+        );
+        return assetBuyer;
+    }


Rather then have the above methods be static methods on AssetBuyer, I think it would be clearer to have then as part of a AssetBuyerFactory module. Thoughts?

I'm personally a fan of static init methods on the class itself but open to changing it if we've established a strong convention of using factories.

fabioberger · 2018-09-20T09:22:49Z

packages/asset-buyer/src/asset_buyer.ts

+    }
+    /**
+     * Get a BuyQuote containing all information relevant to fulfilling a buy.
+     * Pass the BuyQuote to executeBuyQuoteAsync to execute the buy.


For clarity: You can then pass the `BuyQuote` to `executeBuyQuoteAsync` to execute the buy.

fabioberger · 2018-09-20T09:51:39Z

packages/asset-buyer/src/utils/order_fetcher_response_processor.ts

+        orderValidator?: OrderValidatorWrapper,
+    ): Promise<AssetBuyerOrdersAndFillableAmounts> {
+        // drop orders that are expired or not open
+        const filteredTargetOrders = filterOutExpiredAndNonOpenOrders(targetOrderFetcherResponse.orders);


Is there an "expiryBuffer" that users can configure? They might want to be overly cautious, since attempting to fill and expired order will result in them losing gas costs. Since blocks are mined every ~12 seconds, it's hard to predict if an order with 3sec left has a high chance of making it into the next block. Chances are slim but not impossible.

fabioberger · 2018-09-20T09:52:07Z

packages/asset-buyer/src/utils/order_fetcher_response_processor.ts

+        let unsortedFeeOrders = filteredFeeOrders;
+        // if an orderValidator is provided, use on chain information to calculate remaining fillable makerAsset amounts
+        if (!_.isUndefined(orderValidator)) {
+            // TODO: critical


Add author.

fabioberger · 2018-09-20T09:52:23Z

packages/asset-buyer/src/utils/order_fetcher_response_processor.ts

+        // if an orderValidator is provided, use on chain information to calculate remaining fillable makerAsset amounts
+        if (!_.isUndefined(orderValidator)) {
+            // TODO: critical
+            // try catch these requests and throw a more domain specific error


Can you elaborate on this?

I think the comment means that we should be catching the various potential errors from the smart contract wrappers and returning more useful/asset-buyer specific error messages to the users of asset-buyer. However, I reprioritized this to "improvement"

fabioberger · 2018-09-20T09:54:41Z

packages/asset-buyer/src/utils/order_fetcher_response_processor.ts

+        (acc, orderWithAmount) => {
+            const { orders, remainingFillableMakerAssetAmounts } = acc;
+            const { remainingFillableMakerAssetAmount, ...order } = orderWithAmount;
+            // if we are still missing a remainingFillableMakerAssetAmount, assume the order is completely fillable


In which edge-cases would the remainingFillableMakerAssetAmount still be missing at this point?

In practice it wouldn't be, but the type system wants this check since the type can be undefined.

hm, then this comment feels misleading. I'm not a big fan of _.reduce, you could also write this as a simply _.each loop.

fabioberger · 2018-09-20T10:04:11Z

packages/types/CHANGELOG.json

+        "version": "1.1.0",
+        "changes": [
+            {
+                "note": "Add ObjectMap type",


…into feature/forwarder-helper/sra-and-rpc

fabioberger

Please address the remaining changes, then LGTM

fabioberger · 2018-09-24T12:29:28Z

packages/asset-buyer/README.md

+## @0xproject/asset-buyer
+
+Convenience package for buying assets represented on the Ethereum blockchain using 0x. In its simplest form, the package helps in the usage of the [0x forwarder contract](https://github.com/0xProject/0x-protocol-specification/blob/master/v2/forwarder-specification.md), which allows users to execute Wrapped Ether based 0x orders without having to set allowances, wrap Ether or buy ZRX, meaning they can buy tokens with Ether alone. Given some liquidity (0x signed orders), it helps estimate the Ether cost of buying a certain asset (giving a range) and then buying that asset.
+


Let's link Wrapped Ether to https://weth.io/

fabioberger · 2018-09-24T12:30:52Z

packages/asset-buyer/README.md

+
+In its more advanced and useful form, it integrates with the [Standard Relayer API](https://github.com/0xProject/standard-relayer-api) and takes care of sourcing liquidity for you given an SRA compliant endpoint. The final result is a library that tells you what assets are available, provides an Ether based quote for any asset desired, and allows you to buy that asset using Ether alone.
+
+### Read the [Documentation](https://0xproject.com/docs/asset-buyer).


Will there really be a documentation page for this package? If "eventually" is the answer, let's remove this line for now.

fabioberger · 2018-09-24T12:34:30Z

packages/asset-buyer/package.json

-        "@0xproject/typescript-typings": "^2.0.1",
-        "@0xproject/utils": "^1.0.9",
-        "@types/node": "*",
+        "@0xproject/assert": "^1.0.8",


There has been a publish in the mean-time. This should be ^1.0.9 and others are probably also out-of-date.

…into feature/forwarder-helper/sra-and-rpc

fragosti · 2018-09-24T15:47:56Z

Ok we have determined the test-publish job has been failing because of memory issues. So merging.

BMillman19 added WIP labels Aug 28, 2018

BMillman19 requested a review from fragosti August 28, 2018 21:25

BMillman19 requested review from abandeali1 and dekz August 28, 2018 22:25

BMillman19 commented Aug 28, 2018

View reviewed changes

BMillman19 force-pushed the feature/forwarder-helper/sra-and-rpc branch from 118a8be to 44aa369 Compare September 14, 2018 11:43

BMillman19 added 13 commits September 15, 2018 14:14

Add getForwarderHelperForMakerAssetDataAsync method to forwarderHelpe…

8687b95

…rFactory

Remove taker address as param and filter out non-open orders

8d61327

Catch standard relayer api errors

c0a14a4

Various clean up

90674d9

Move packages/forwarder-helper into packages/asset-buyer

91702bb

Update readme, changelog, and package.json with rename

8da7d39

Create initial AssetBuyer class

7b46cef

Flesh out the AssetBuyer class

f1a22e9

Implement StandardRelayerAPIOrderFetcher

190bf25

Implement ProvidedOrderFetcher

4e59be9

Delete old test

dfe58e4

Fix lint errors

c692115

Export the order fetchers

d57619b

BMillman19 force-pushed the feature/forwarder-helper/sra-and-rpc branch from 44aa369 to d57619b Compare September 15, 2018 12:26

BMillman19 changed the title ~~[WIP][forwarder-helper]Add getForwarderHelperForMakerAssetDataAsync method to forwarderHelperFactory~~ [WIP][asset-buyer] Create new AssetBuyer class Sep 15, 2018

BMillman19 added @0x/asset-buyer and removed forwarder-helper labels Sep 15, 2018

BMillman19 added 3 commits September 15, 2018 14:47

Add factory method on AssetBuyer for provided orders

af40989

Add factory method on AssetBuyer for specific token address

9ec2b5a

Add factory method for specific assetData to buy and add comments

a22d2dc

fragosti reviewed Sep 17, 2018

View reviewed changes

fragosti added 7 commits September 18, 2018 16:42

make the slippage percentage customizable by integrator

0003666

Make ForwaderWrapper take in a number for feePercentage instead of Bi…

fd4b4f8

…gNumber

Add isValidPercentage assert

93f7e33

Calculate min and max rates in buy quote

60e2dfd

Rename forceOrderRefresh to shouldForceOrderRefresh

14345ab

Move ObjectMap to shared types

a03b1d4

Implement StandardRelayerAPIAssetBuyerManager

a44f77a

fabioberger suggested changes Sep 20, 2018

View reviewed changes

fragosti added 12 commits September 20, 2018 14:09

Add better description to README

3238925

Update descriptions for methods on AssetBuyer

d23487b

Rename OrderFetcher to OrderProvider and other small improvements

03e18ff

Add utility to get assetDatas

35c324f

Move some logic to order utils

fcca63a

Add labels to TODOs

97150cf

Renamed orderFetcherResponseProcessor to orderProviderResponseProcessoer

8dea48e

More renaming

c64dcec

Make asset buyer map private

c48cf3a

Add order provider response validation

1bfaefb

Small typos

5e84e96

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

7a53766

…into feature/forwarder-helper/sra-and-rpc

fragosti removed the WIP label Sep 21, 2018

fragosti changed the title ~~[WIP][asset-buyer] Create new AssetBuyer class~~ [asset-buyer] Create new AssetBuyer class Sep 21, 2018

Add expiry buffer

e4b664b

fabioberger approved these changes Sep 24, 2018

View reviewed changes

fragosti added 3 commits September 24, 2018 15:43

Improve README

7a43e19

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

f49b231

…into feature/forwarder-helper/sra-and-rpc

Upgrade all deps

d8d1c98

fragosti merged commit b830c28 into development Sep 24, 2018

fragosti deleted the feature/forwarder-helper/sra-and-rpc branch September 24, 2018 15:48


		Provides convenience objects to help work with the Forwarder Contract
		Convenience package for buying assets

		## @0xproject/asset-buyer

		Convenience package for buying assets represented on the Ethereum blockchain using 0x. In its simplest form, the package helps in the usage of the [0x forwarder contract](https://github.com/0xProject/0x-protocol-specification/blob/master/v2/forwarder-specification.md), which allows users to execute Wrapped Ether based 0x orders without having to set allowances, wrap Ether or buy ZRX, meaning they can buy tokens with Ether alone. Given some liquidity (0x signed orders), it helps estimate the Ether cost of buying a certain asset (giving a range) and then buying that asset.


		In its more advanced and useful form, it integrates with the [Standard Relayer API](https://github.com/0xProject/standard-relayer-api) and takes care of sourcing liquidity for you given an SRA compliant endpoint. The final result is a library that tells you what assets are available, provides an Ether based quote for any asset desired, and allows you to buy that asset using Ether alone.

		### Read the [Documentation](https://0xproject.com/docs/asset-buyer).

[asset-buyer] Create new AssetBuyer class #1037

[asset-buyer] Create new AssetBuyer class #1037

Conversation

BMillman19 commented Aug 28, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Aug 28, 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

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

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

fabioberger 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

fragosti commented Sep 24, 2018

BMillman19 commented Aug 28, 2018 •

edited

Loading

coveralls commented Aug 28, 2018 •

edited

Loading