Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Asset-swapper aggregator utils #2353

Merged
merged 34 commits into from
Dec 16, 2019

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented Nov 20, 2019

yoink

Description

Adds aggregator utils to @0x/asset-swapper, which can be found in src/utils/aggregate.ts.

Improving Quotes

The main functions of interest are improveMarketSellAsync()and improveMarketBuyAsync(). Each takes native orders, a fill amount, and some options. They will poll on-chain DEXes and return a mixture of native and bridge orders that can be passed into marketBuy() or marketFill() on the Exchange contract.

/**
 * Improve a market sell operation by (potentially) merging native orders with
 * generated bridge orders.
 * @param samplerContract The `IERC20BridgeSamplerContract` contract.
 * @param nativeOrders Native orders.
 * @param takerAmount Amount of taker asset to sell.
 * @param opts Options object.
 * @return Improved orders.
 */
export async function improveMarketSellAsync(
    samplerContract: IERC20BridgeSamplerContract,
    nativeOrders: SignedOrderWithoutDomain[],
    takerAmount: BigNumber,
    opts?: Partial<ImproveOrdersOpts>,
): Promise<SignedOrderWithoutDomain[]>;

/**
 * Improve a market buy operation by (potentially) merging native orders with
 * generated bridge orders.
 * @param samplerContract The `IERC20BridgeSamplerContract` contract.
 * @param nativeOrders Native orders.
 * @param makerAmount Amount of maker asset to buy.
 * @param opts Options object.
 * @return Improved orders.
 */
export async function improveMarketBuyAsync(
    samplerContract: IERC20BridgeSamplerContract,
    nativeOrders: SignedOrderWithoutDomain[],
    makerAmount: BigNumber,
    opts?: Partial<ImproveOrdersOpts>,
): Promise<SignedOrderWithoutDomain[]>;

Options

An options object can be passed into either function, which control slippage on bridge orders, sources used, and performance of the mixing algorithm.

/**
 * Options for `improveMarketBuyAsync()` and `improveMarketSellAsync()`.
 */
export interface ImproveOrdersOpts {
    /**
     * Liquidity sources to exclude. Default is none.
     */
    excludedSources: ERC20BridgeSource[];
    /**
     * Whether to prevent mixing Kyber orders with Uniswap and Eth2Dai orders.
     */
    noConflicts: boolean;
    /**
     * Complexity limit on the search algorithm, i.e., maximum number of
     * nodes to visit. Default is 1024.
     */
    runLimit: number;
    /**
     * When generating bridge orders, we use
     * sampled rate * (1 - bridgeSlippage)
     * as the rate for calculating maker/taker asset amounts.
     * This should be a small positive number (e.g., 0.0005) to make up for
     * small discrepancies between samples and truth.
     * Default is 0.0005 (5 basis points).
     */
    bridgeSlippage: number;
    /**
     * Number of samples to take for each DEX quote.
     */
    numSamples: number;
    /**
     * Dust amount, as a fraction of the fill amount.
     * Default is 0.01 (100 basis points).
     */
    dustThreshold: number;
}

Changes to the ERC20BridgeSampler contract.

Previously, queryOrdersAndSampleSells/Buys() would return the OrderInfo for each order passed in, along with DEX samples. Now it calls DevUtils.getOrderRelevantStates() and returns the actual fillable maker or taker amount (depending on whether it's a buy or sell). Also, if the order status is not FILLABLE or the signature is invalid, this amount will be zero.

TODO

  • Fold this library into asset-swapper's quoting logic.
  • The addresses of the bridge contracts in aggregate.ts are wrong and will need to be updated once the final versions of the bridges have been deployed.
  • Incorporate native order fees
  • Fix outstanding/breaking TODOs (mainly the redundant work)
  • Fix test coverage
  • Add Sampler to @0x/contract-addresses
  • If no nativeOrders pass in a "dummy order" such that swapper now uses only other DEX sources to prepare quote.
  • Gas is not accounted for. Not sure how we want to deal with these.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 4f93259 to e94dc90 Compare November 20, 2019 03:23
@buildsize
Copy link

buildsize bot commented Nov 25, 2019

File name Previous Size New Size Change
init.py 60.25 KB 60.25 KB 0 bytes (0%)
abi_gen_dummy.ts 77.76 KB [deleted]
lib_dummy.ts 7.13 KB [deleted]
test_lib_dummy.ts 10.38 KB [deleted]
environment.pickle 1.61 MB 1.61 MB 174 bytes (0%)
index.doctree 187.37 KB 187.6 KB 230 bytes (0%)
.buildinfo 230 bytes 230 bytes 0 bytes (0%)
genindex.html 5.6 KB 5.6 KB 0 bytes (0%)
index.html 2.52 KB 2.52 KB 0 bytes (0%)
objects.inv 375 bytes 375 bytes 0 bytes (0%)
py-modindex.html 3.07 KB 3.07 KB 0 bytes (0%)
search.html 2.84 KB 2.84 KB 0 bytes (0%)
searchindex.js 6.31 KB 6.31 KB 0 bytes (0%)
index.rst.txt 415 bytes 415 bytes 0 bytes (0%)
alabaster.css 10.92 KB 10.92 KB 0 bytes (0%)
basic.css 11.89 KB 11.97 KB 90 bytes (1%)
custom.css 42 bytes 42 bytes 0 bytes (0%)
doctools.js 9.05 KB 9.05 KB 0 bytes (0%)
documentation_options.js 303 bytes 324 bytes 21 bytes (7%)
file.png 286 bytes 286 bytes 0 bytes (0%)
jquery-[version].js 273.79 KB 273.79 KB 0 bytes (0%)
jquery.js 86.08 KB 86.08 KB 0 bytes (0%)
language_data.js 10.59 KB 10.59 KB 0 bytes (0%)
minus.png 90 bytes 90 bytes 0 bytes (0%)
plus.png 90 bytes 90 bytes 0 bytes (0%)
pygments.css 4.69 KB 4.69 KB 0 bytes (0%)
searchtools.js 15.61 KB 15.65 KB 42 bytes (0%)
underscore-[version].js 34.34 KB 34.34 KB 0 bytes (0%)
underscore.js 11.86 KB 11.86 KB 0 bytes (0%)
contract_addresses.html 16.8 KB 16.8 KB 0 bytes (0%)
contract_artifacts.html 8.24 KB 8.24 KB 0 bytes (0%)
json_schemas.html 12.56 KB 12.56 KB 0 bytes (0%)
order_utils.html 47.16 KB 47.16 KB 0 bytes (0%)
erc20_token.html 93.56 KB 93.56 KB 0 bytes (0%)
exchange.html 718.38 KB 718.38 KB 0 bytes (0%)
tx_params.html 9.41 KB 9.41 KB 0 bytes (0%)
local_message_signer.html 15.08 KB 15.08 KB 0 bytes (0%)
asset_data_utils.html 22.66 KB 22.66 KB 0 bytes (0%)
default_api.html 113.17 KB 113.17 KB 0 bytes (0%)
asset_proxy_owner.html 337.39 KB 337.39 KB 0 bytes (0%)
coordinator.html 128.72 KB 128.72 KB 0 bytes (0%)
coordinator_registry.html 39.59 KB 39.59 KB 0 bytes (0%)
dutch_auction.html 66.13 KB 66.13 KB 0 bytes (0%)
erc20_proxy.html 109.07 KB 109.07 KB 0 bytes (0%)
erc721_proxy.html 109.19 KB 109.19 KB 0 bytes (0%)
erc721_token.html 150.2 KB 150.2 KB 0 bytes (0%)
forwarder.html 124.01 KB 124.01 KB 0 bytes (0%)
i_asset_proxy.html 40.18 KB 40.18 KB 0 bytes (0%)
i_validator.html 27.06 KB 27.06 KB 0 bytes (0%)
i_wallet.html 24.9 KB 24.9 KB 0 bytes (0%)
multi_asset_proxy.html 144.04 KB 144.04 KB 0 bytes (0%)
order_validator.html 107.69 KB 107.69 KB 0 bytes (0%)
weth9.html 132.09 KB 132.09 KB 0 bytes (0%)
zrx_token.html 107.61 KB 107.61 KB 0 bytes (0%)
dev_utils.html 526.89 KB 526.89 KB 0 bytes (0%)
types.html 8.54 KB 8.54 KB 0 bytes (0%)
erc1155_mintable.html 276.51 KB 276.51 KB 0 bytes (0%)
erc1155_proxy.html 130.38 KB 130.38 KB 0 bytes (0%)
static_call_proxy.html 34.04 KB 34.04 KB 0 bytes (0%)

@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 6170fbd to b2ac850 Compare November 26, 2019 20:46
@coveralls
Copy link

coveralls commented Nov 26, 2019

Coverage Status

Coverage remained the same at 77.804% when pulling 12f8cb0 on feat/asset-swapper/ERC20BridgeAggregator into 6808e0d on development.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from b2ac850 to e3d6017 Compare November 26, 2019 21:07
@dorothy-zbornak dorothy-zbornak marked this pull request as ready for review November 26, 2019 21:07
@dorothy-zbornak dorothy-zbornak requested review from dave4506 and removed request for dave4506 November 26, 2019 21:07
@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from e3d6017 to de451c7 Compare November 26, 2019 21:12
@dorothy-zbornak dorothy-zbornak changed the title [WIP] asset-swapper aggregator utils asset-swapper aggregator utils Nov 26, 2019
...IMPROVE_ORDERS_OPTS_DEFAULTS,
...opts,
};
const [orderInfos, dexQuotes] = await queryNetworkAsync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on using DevUtils.getOrderRelevantState in the Sampler and returning [orderInfos, fillableTakerAssetAmount, isValidSignature].

We perform this to ensure the order is valid and check how much is fillable. We could skip that check a by combining it in the Sampler and reduce an RPC call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm down for something like that. It looks like there's some more complex pruning rules being applied. So maybe the sampler contract can query those states and pass them into an orderFilter callback in opts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, really all we care about is how much taker asset is fillable. I think I'll just have the contract return that quantity, which will also be zero if the signature is invalid or if the order status is not FILLABLE.

packages/asset-swapper/src/utils/aggregate.ts Outdated Show resolved Hide resolved
if (!provider) {
throw new Error(AggregationError.MissingProvider);
}
const sampler = new ERC20BridgeSamplerContract(SAMPLER_ADDRESS, provider);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd even consider just passing in the entire ERC20BridgeSampler, won't have to deal with optional providers here or in ImproveOrdersOpts

/**
* Create ERC20Proxy asset data.
*/
export function createERC20AssetData(tokenAddress: string): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can re-use DevUtils here which performs this pure in memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate DevUtils but OK 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, how about we wait until DevUtils is sorted out since switching this one function won't really do me any favors right now.

/**
* Create ERC20Bridge asset data.
*/
export function createBridgeAssetData(tokenAddress: string, bridgeAddress: string, bridgeData: string): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an asana task to get this encoding/decoding into DevUtils and re-use from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(if it fits)

@@ -43,6 +43,7 @@
"@0x/assert": "^2.2.0-beta.2",
"@0x/contract-addresses": "^3.3.0-beta.4",
"@0x/abi-gen-wrappers": "^5.4.0-beta.3",
"@0x/contracts-erc20-bridge-sampler": "^1.0.0-beta.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO add this to abi-gen-wrappers/export from contract-wrappers. These have pruned artifacts and won't bloat out the asset-swapper bundle size.

nativeOrders: OrderWithoutDomain[],
takerAmount: BigNumber,
opts?: Partial<ImproveOrdersOpts>,
): Promise<OrderWithoutDomain[]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct in terms of turning it into a WalletSignature?

        const improvedOrders = await improveMarketBuyAsync(prunedOrders, assetFillAmount, {
            provider: this.provider,
        });
        const improvedSignedOrders = improvedOrders.map(i => ({
            ...i,
            signature: '0x04',
            chainId: this.chainId,
            exchangeAddress: this._contractAddresses.exchange,
        }));

IIRC

Wallet = 0x04
EIP1271Wallet = 0x07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yap!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for the bridge orders. You still have to map the native orders to their respective (probably non-Wallet) signatures. I'll handle signatures in the upcoming iteration, though, so it'll be moot.

opts?: Partial<ImproveOrdersOpts>,
): Promise<OrderWithoutDomain[]> {
if (nativeOrders.length === 0) {
throw new Error(AggregationError.EmptyOrders);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the flow for just retrieving a bunch of BridgeOrders if there are no NativeOrders for this asset pair?

Just use sampleBuys/sampleSells directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just pass in an empty order to keep the entry point the same.

@@ -0,0 +1,800 @@
import { IERC20BridgeSamplerContract } from '@0x/contracts-erc20-bridge-sampler';
Copy link
Contributor

@dave4506 dave4506 Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dekz highlighted above, but will need to rebase this PR with development we switched back to using @0x/contract-wrappers for bundle size reasons.

Copy link
Contributor

@dave4506 dave4506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! Only minor feedback on the design of the algorithm.

Thought though: @dekz @dorothy-zbornak
While this algorithm searches for optimal paths with random walks, can we optimize further with a linear algebra interpretation of the paths?

I am thinking that with some pruning we can get a rather sparse matrix representation of the edges and Fill Paths, with some modified form of dijkstra's algorithm or some other optimization algorithm and an efficient sparse matrix typescript library, we may be able to see performance gains?

Just a shower thought, maybe for a later upgrade if performance becomes an issue.

import { constants } from '../constants';

const { NULL_BYTES, NULL_ADDRESS } = constants;
const ZERO = new BigNumber(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. these should be constants.ts

Copy link
Contributor Author

@dorothy-zbornak dorothy-zbornak Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`Eh?~ nvm

parent?: Fill;
// Arbitrary data to store in this Fill object. Used to reconstruct orders
// from paths.
data?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like a more verbose name here other than data would help

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually just be FillData, if we need more data entries, we can always add it.

We can use a union type to capture all of the 'types' that data can be.

* Dust amount, as a fraction of the fill amount.
* Default is 0.01 (100 basis points).
*/
dustThreshold: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. prefer dustAmountThreshhold

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, we seem to use amount for flat amounts, whereas this is a %. Maybe dustFractionThreshold?

return path;
}

function createBuyPathFromNativeOrders(orders: SignedOrderWithoutDomain[], fillableAmounts: BigNumber[]): Fill[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse some of the logic from above? (as in dedup the logic) Not a big issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out these are subtly different enough to make it not worth it.

currentPathOutput: ZERO,
currentPathFlags: 0,
};
// Visit all valid combintations of fills to find the optimal path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Visit all valid combintations of fills to find the optimal path.
// Visit all valid combinations of fills to find the optimal path.

const optimalPath = optimizer.optimize(
sortFillsByPrice(allFills),
takerAmount,
pickOptimalPath(allPaths, takerAmount),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to move this pickOptimalPath logic into FillsOptimizer? Seems like it makes more sense there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pickOptimalPath just picks the shortest path of those passed in, but FillsOptimizer's job is to actually generate new paths so idk if that's a great fit.

* Convert a source to a canonical address used by the sampler contract.
*/
export const SOURCE_TO_ADDRESS: { [key: string]: string } = {
[ERC20BridgeSource.Eth2Dai]: '0x39755357759cE0d7f32dC8dC45414CCa409AE24e',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would y'all feel about putting these in a central location, like contract-addresses? (or even just a JSON file in here called contract-addresses.json with mappings for the various networks)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for sure, once these are actually all deployed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized these are DEX addresses. Yeah, idk, feels weird to put non 0x stuff in contract-addresses. 🤔

/**
* Class for finding optimized fill paths.
*/
export class FillsOptimizer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that AMM's like the one used by Uniswap will give you a worse price the more you fill. Is this the case for all of our sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule, yes. In practice, we often see quotes up to and beyond $10k, for high liquidity pairs, that are virtually linear (i.e., rates are essentially constant).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as a rule except for maybe Kyber, which is why we fall back to the flood fill.

@dorothy-zbornak
Copy link
Contributor Author

This looks awesome! Only minor feedback on the design of the algorithm.

Thought though: @dekz @dorothy-zbornak
While this algorithm searches for optimal paths with random walks, can we optimize further with a linear algebra interpretation of the paths?

I am thinking that with some pruning we can get a rather sparse matrix representation of the edges and Fill Paths, with some modified form of dijkstra's algorithm or some other optimization algorithm and an efficient sparse matrix typescript library, we may be able to see performance gains?

Just a shower thought, maybe for a later upgrade if performance becomes an issue.

Yeah, there is no doubt we can optimize this algo to reduce the space complexity. The pruning done here is not super aggressive. One obvious one we can do for buys is avoid traversing paths that would exceed the global minimum cost. There are some issues with using LA to find ideal splits, like not being able to partially fill orders (except for the last) with market functions, so we might have to go down the route of having a custom forwarder for asset-swapper.

Another thing worth noting is that in most cases (knock on wood), this algo does approximate the globally optimal solution early on, because the solution for combining convex quotes should just be discoverable using a simple greedy algorithm. Because the fill nodes are sorted by price, this solution is walked over sooner rather than later.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from b0bd256 to eece31f Compare December 4, 2019 00:58
@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch 2 times, most recently from f06b7b9 to fc84a08 Compare December 4, 2019 05:18
@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from fc84a08 to 1e2de2a Compare December 4, 2019 06:28
@dave4506 dave4506 force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 1e2de2a to 8389937 Compare December 6, 2019 03:22
@dave4506 dave4506 changed the title asset-swapper aggregator utils [WIP] asset-swapper aggregator utils Dec 6, 2019
@dave4506
Copy link
Contributor

dave4506 commented Dec 6, 2019

Adding back the [WIP] tag, to finish the work with integrating asset-swapper with the aggregator utils and wrapping up the TODO tasks.

@dave4506 dave4506 removed the request for review from albrow December 12, 2019 20:00
@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from d81bac0 to c135945 Compare December 14, 2019 01:32
},
{
"note": "Add `IERC20BridgeSampler` wrapper",
"pr": 2353
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a new entry; current entry is already published

@@ -5,6 +5,10 @@
{
"note": "Add `IAssetData` artifact",
"pr": 2373
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a new entry right? current entry is already published

@@ -14,6 +14,10 @@
{
"note": "Add `ERC20BridgeAssetData`",
"pr": 2373
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same; this needs a new entry

@dekz
Copy link
Member

dekz commented Dec 15, 2019

Thanks for catching that @xianny. I pushed a fix to that.

@dekz dekz force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 97fdc35 to f49cb9a Compare December 15, 2019 02:24
@dekz dekz force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from f49cb9a to 72be518 Compare December 15, 2019 02:25
@dave4506 dave4506 changed the title [WIP] asset-swapper aggregator utils Asset-swapper aggregator utils Dec 15, 2019
@dekz dekz force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 34447d2 to 13fc6a7 Compare December 15, 2019 05:17
Copy link
Member

@dekz dekz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I think we can do a follow up clean up/refactor once this is shipped. Most of my comments are subjective styling.

One refactor which would be great to have is in the tests which are pretty complex to follow (especially with the dependance on ERC20BridgeSource being a num enum.

From a blackbox perspective this has been performing very competitively, so mad props to @dave4506 and @dorothy-zbornak.

@@ -1,41 +1,73 @@
import { Order } from '@0x/types';
import { BigNumber } from '@0x/utils';
import * as heartbeats from 'heartbeats';
import * as _ from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not using lodash in this file.

Suggested change
import * as _ from 'lodash';

// tslint:disable-next-line:prefer-function-over-method
public async getProtocolFeeMultiplierAsync(): Promise<BigNumber> {
public getProtocolFeeMultiplier(): BigNumber {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider keeping this async for the future, we might want to poll every few minutes. Keeping it async now would prevent a breaking future change. Not a huge deal though.

};
}

function getSourceFromAssetData(assetData: string): ERC20BridgeSource {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a useful function for data purposes (determining the composition). We should move it into utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not this version though; it's not very thorough.

function getSourceFromAddress(sourceAddress: string): ERC20BridgeSource {
for (const k of Object.keys(SOURCE_TO_ADDRESS)) {
if (SOURCE_TO_ADDRESS[k].toLowerCase() === sourceAddress.toLowerCase()) {
return parseInt(k, 10) as ERC20BridgeSource;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO I don't think we should rely on the enum default ints. It's definitely bitten me before, i.e

if (ERC20BridgeSource.Native) // if (0)

There is a lot of logic dependent on this though so let's have a TODO item to replace this with String enums

queryOrdersAndSampleBuys: (orders, signatures, sources, fillAmounts) => [
orders.map(o => o.makerAssetAmount),
sources.map(s =>
shortZip(fillAmounts, rates[getSourceFromAddress(s)]).map(([f, r]) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hurt my brain 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rewrite this to be clearer.

});
}

function getOrderTokens(order: SignedOrder): [string, string] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO assetDataUtils is back

* Common exception messages thrown by aggregation logic.
*/
export enum AggregationError {
NoOptimalPath = 'no optimal path',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NoOptimalPath = 'no optimal path',
NoOptimalPath = 'NO_OPTIMAL_PATH',

*/
export enum AggregationError {
NoOptimalPath = 'no optimal path',
EmptyOrders = 'empty orders',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EmptyOrders = 'empty orders',
EmptyOrders = 'EMPTY_ORDERS',

public async getSignedOrdersWithFillableAmountsAsync(
signedOrders: SignedOrder[],
): Promise<SignedOrderWithFillableAmounts[]> {
const signatures = _.map(signedOrders, o => o.signature);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const signatures = _.map(signedOrders, o => o.signature);
const signatures = signedOrders.map(o => o.signature);

import { DevUtilsContract } from '@0x/contract-wrappers';
import { orderCalculationUtils } from '@0x/order-utils';
import { OrderStatus, SignedOrder } from '@0x/types';
import * as _ from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import * as _ from 'lodash';

@dorothy-zbornak dorothy-zbornak force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from 7a24477 to 7d02e54 Compare December 16, 2019 08:09
Copy link
Contributor Author

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed most of @dekz's comments. I think this is in a pretty good place now. Awesome work, @dave4506. Would approve this PR if I was allowed to.

* Create ERC20Bridge asset data.
*/
export function createBridgeAssetData(tokenAddress: string, bridgeAddress: string, bridgeData: string): string {
const encoder = AbiEncoder.createMethod('ERC20Bridge', [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO once we get bridge stuff in assetDataUtils.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already in there :)

@xianny xianny force-pushed the feat/asset-swapper/ERC20BridgeAggregator branch from b4292b8 to 12f8cb0 Compare December 16, 2019 19:54
@xianny xianny merged commit 9949085 into development Dec 16, 2019
@xianny xianny deleted the feat/asset-swapper/ERC20BridgeAggregator branch December 16, 2019 20:36
dorothy-zbornak added a commit that referenced this pull request Feb 27, 2020
* `@0x/asset-swapper`: Add ERC20Bridge aggregator library.

* `@0x/asset-swapper`: Finish off `aggregate.ts`.

* `@0x/types`: Add `OrderWithoutDomain` type.

* `@0x/asset-swapper`: Add testing infra for sampler/aggregator.

* `@0x/types`: Add `SignedOrderWithoutDomain` type.

* `@0x/asset-swapper`: Update aggregator to take and return orders with signatures.

* `@0x/asset-swapper`: Fix broken aggregator tests.

* `@0x/asset-swapper`: Pass the sampler contract into aggregator entry points.

* `@0x/contract-artifacts`: Add `IERC20BridgeSampler` artifact.

* `@0x/contract-wrappers`: Add `IERC20BridgeSampler` wrapper.

* `@0x/asset-swapper`: Address review comments.

* fixed testing

* refactored aggregate.ts and embeded into asset-swapper

* added adjusted rates for taker and maker fees

* remove PrunedSignedOrders

* updated contract-addresses and addressed some other todos

* streamlined logic

* patched in lawrences changes

* renamed aggregator utils and removed market_utils.ts

* added ack heartbeats

* fixed bug

* patches

* added dummy order things

* Dummy with valid sig

* Tweak gas price calculation to wei

* added test coverage and fixed bugs

* fixed migrations

* Fix CHANGELOGs and types export

* Deploy latest ERC20BridgeSampler on Mainnet

* `@0x/types` Revert CHANGELOG.

* `@0x/asset-swapper`: Address review comments.
`@0x/contract-addresses`: Make kyber lowercase.

* made protocol fee multiplier async

* `@0x/asset-swapper: Fix build errors and do some code cleanup.

* use assetDataUtils where possible
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants