BalancerBridge #2613

moodlezoup · 2020-06-26T23:15:13Z

Description

Adds the BalancerBridge contract and corresponding AssetSwapper support.
AssetSwapper uses the Balancer SOR package to get pool addresses and compute quotes off-chain.
Also includes some related AssetSwapper refactors, most notably merging all Curves into a single ERC20BridgeSource. To account for the fact that different Curves have significantly different gas costs, we change the fee schedule so that each source is given a fee estimate function instead of a constant.

Testing instructions

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

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

hysz

I don't have full context on the client-side code but the Solidity LGTM! We should probably add some basic tests before merging. Esp since simbot is not currently part of the continuous integration tests, we'll want some kind of sanity checks in here to future proof the implementation.

packages/asset-swapper/src/utils/market_operation_utils/curve_utils.ts

xianny

nice job with the multiple quotes per source!!

dekz · 2020-07-02T00:30:47Z

contracts/asset-proxy/contracts/src/bridges/BalancerBridge.sol

+        bytes calldata bridgeData
+    )
+        external
+        freesGasTokensFromCollector


This isn't needed and I should really roll back the change in Curve Bridge development. We technically downgraded the CurveBridge address to the older non-burning equivalent when we shipped DFB.

We now either free from DFB or in the future FQT.

packages/asset-swapper/src/utils/market_operation_utils/constants.ts

dekz · 2020-07-02T00:42:54Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

-        case ERC20BridgeSource.CurveUsdcDaiUsdtTusd:
-        case ERC20BridgeSource.CurveUsdcDaiUsdtBusd:
-        case ERC20BridgeSource.CurveUsdcDaiUsdtSusd:
+        case ERC20BridgeSource.Curve:


@johnrjj FYI Sources might start coming back as Curve instead of Curve_DAI_USDC and UniswapV2 instead of UniswapV2_ETH. Was the change made in matcha to split on _ and take the first element?

dekz · 2020-07-02T00:44:44Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+                    curveFillData.poolAddress,
+                    curveFillData.fromTokenIdx,
+                    curveFillData.toTokenIdx,
+                    1,


One day I'll remove this version hack. Then Ren BTC Curve introduces a new "version" 😂

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

dorothy-zbornak

Really digging this refactor. Hopefully this leads to many more.

dorothy-zbornak · 2020-07-02T01:45:54Z

contracts/integrations/test/bridges/balancer_bridge_mainnet_test.ts

+            await testContract
+                .bridgeTransferFrom(usdcAddress, constants.NULL_ADDRESS, receiver, constants.ZERO_AMOUNT, bridgeData)
+                .awaitTransactionSuccessAsync({ from: CHONKY_WETH_WALLET, gasPrice: 0 }, { shouldValidate: false });
+        });


Should these tests check that the receiver actually received something?

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

packages/asset-swapper/src/utils/market_operation_utils/curve_utils.ts

packages/asset-swapper/src/utils/market_operation_utils/types.ts

dorothy-zbornak · 2020-07-02T04:10:46Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

+    FakeBuyOpts,
+    SourceQuoteOperation,
+    UniswapV2FillData,
+} from './types';

 /**


Starting to feel like this needs to be a class so it can hold state like registry addresses and cached balancer pools and calls to some of these ops can be more legible. I think it'll clean up some of the tests as well.

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

packages/asset-swapper/src/utils/market_operation_utils/types.ts

dorothy-zbornak

still going through tests

dorothy-zbornak · 2020-07-08T01:54:29Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

                this._wethAddress,
+                this._sampler.balancerPoolsCache,
                this._liquidityProviderRegistry,
                this._multiBridge,


this arg list getting ridic
def need to make DexOrderSampler ops a class during refactors

dorothy-zbornak · 2020-07-08T02:19:21Z

packages/asset-swapper/src/utils/quote_simulation.ts

@@ -148,12 +145,12 @@ export function fillQuoteOrders(
        if (remainingInput.lte(0)) {
            break;
        }
+        result.gas += getTotalGasUsedByFills(fo.order.fills, gasSchedule);


Why was this calculation moved here? Isn't this wrong? We might not need to hit all the fills/sources.

oops, you're right

dorothy-zbornak · 2020-07-08T02:39:35Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+    public async getPoolsForPairAsync(takerToken: string, makerToken: string): Promise<BalancerPool[]> {
+        const key = JSON.stringify([takerToken, makerToken].sort());
+        const value = this._cache[key];
+        const minTimestamp = new Date().getTime() - this.cacheExpiryMs;


Suggested change

const minTimestamp = new Date().getTime() - this.cacheExpiryMs;

const minTimestamp = Date.now() - this.cacheExpiryMs;

dorothy-zbornak · 2020-07-08T02:40:32Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+        const minTimestamp = new Date().getTime() - this.cacheExpiryMs;
+        if (value.timestamp < minTimestamp) {
+            const pools = await this._fetchPoolsForPairAsync(takerToken, makerToken);
+            const timestamp = new Date().getTime();


Suggested change

const timestamp = new Date().getTime();

const timestamp = Date.now();

dekz

Also seeing a relatively high revert rate.

Some of them are likely explained in the routing being very similar to the following:

	Uniswap: 99.99%, Uniswap_V2: 6e-16%

Definitely not worth filling Uniswap_V2 for a tiny fraction of a $600 trade.

Below is what I'm seeing locally over MKR,USDC,DAI,WETH,WBTC.

dekz · 2020-07-08T07:44:50Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+        const value = this._cache[key];
+        const minTimestamp = Date.now() - this.cacheExpiryMs;
+        if (value === undefined || value.timestamp < minTimestamp) {
+            const pools = await this._fetchPoolsForPairAsync(takerToken, makerToken);


Should we keep only the top 10 pools when sorted by in/out balances?

yeah probably –– I guess top 10 by out balances makes sense?

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

dekz · 2020-07-08T07:56:51Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

                difference(FEE_QUOTE_SOURCES.concat(this._optionalSources()), _opts.excludedSources),
                makerToken,
                this._wethAddress,
                ONE_ETHER,
                this._wethAddress,
+                this._sampler.balancerPoolsCache,


tbh this probably doesn't need to be a parameter and can be found out of global space?
Like BalancerPoolCache.getInstance().blah( the others are parameters as they change on network or are provided as variables ALL the way down the stack. This feels less like that?

Maybe it's easier to test when it's injected in but I have a feeling we aren't testing this explicitly.

dekz · 2020-07-08T07:58:53Z

packages/asset-swapper/src/utils/market_operation_utils/sampler.ts

+    constructor(
+        private readonly _samplerContract: IERC20BridgeSamplerContract,
+        public balancerPoolsCache: BalancerPoolsCache = new BalancerPoolsCache(),
+    ) {}


same comment as above imo

dekz · 2020-07-08T08:56:33Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+        public cacheExpiryMs: number = THIRTY_MINUTES_MS,
+    ) {}
+    public async getPoolsForPairAsync(takerToken: string, makerToken: string): Promise<BalancerPool[]> {
+        const key = JSON.stringify([takerToken, makerToken].sort());


Suggested change

const key = JSON.stringify([takerToken, makerToken].sort());

const key = JSON.stringify([takerToken, makerToken]);

is balanceIn relative to the first argument in fetchPoolsForPair? And sorting these would result in an invalid rate in the opposite direction?

E.g

query DAI, WETH, balanceIn is DAI. Result is stored as DAI, WETH.
query WETH, DAI, balanceIn is WETH. Result is already stored and retrieved from DAI, WETH`

yep, you're right

dekz · 2020-07-08T09:59:52Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+    timestamp: number;
+    pools: BalancerPool[];
+}
+const THIRTY_MINUTES_MS = 30 * 60 * 1000; // tslint:disable-line:custom-no-magic-numbers


IMO 30 mins is fine for a cache of existing pools. I.e a new pool added takes 30m till it is served by AssetSwapper.

30 mins is too long for rates within the pool and determining the route based on these values. Especially with sub 1% slippage.

I'd suggest we cache for at most a minute and move towards a model where the pools are discovered (cached) but queried on-demand.

Agreed. I do wonder if it'd be worth differentiating pool discovery vs pool state. idk if there would be a substantial difference in latency between querying the state of the cached pool addresses versus calling getPoolsWithTokens

yeah I assume TheGraph is fairly quick, which is fine for pool discovery.

But ultimately I think we'd want to get the rates as live as possible to reduce our potential for revert.

dorothy-zbornak · 2020-07-10T07:18:31Z

packages/utils/src/configured_bignumber.ts

@@ -34,4 +34,7 @@ if (isNode) {
    };
 }

+const orig = BigNumber.config;
+BigNumber.config = (_args?: any) => orig({});


Should probably document why we do this and also clobber BigNumber.set().

Also, maybe this is neater?

(orig => BigNumber.config = (...args: any) => orig({}))(BigNumber.config); BigNumber.set = BigNumber.config;

dorothy-zbornak · 2020-07-10T07:29:49Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

+        const samplerOps = subOps.filter(op => op.source !== ERC20BridgeSource.Balancer);
+        const nonSamplerOps = subOps.filter(op => op.source === ERC20BridgeSource.Balancer);


Is this change just to reduce the load on the sampler contract?

the sampler was reverting because batchCall didn't know what to do with empty bytes. we could change batchCall instead to skip over if callDatas[i] == 0, but 🤷

dorothy-zbornak · 2020-07-10T07:33:12Z

packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts

+                b.balanceOut.minus(a.balanceOut).toNumber(),
+            );
+            // tslint:disable-next-line:custom-no-magic-numbers
+            return pools.length > 10 ? pools.slice(0, 10) : pools;


I think it's worth putting 10 into a constant.

… balance

dorothy-zbornak

Looking good. Can backlog my comments if you're under pressure.

dorothy-zbornak · 2020-07-14T21:41:50Z

packages/asset-swapper/src/utils/market_operation_utils/path_optimizer.ts

 import { Fill } from './types';

 // tslint:disable: prefer-for-of custom-no-magic-numbers completed-docs

+const RUN_LIMIT_DECAY_FACTOR = 0.8;


Wonder how detrimental 0.5 would be...

dorothy-zbornak · 2020-07-14T21:50:13Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

        const rfqtPromise = getRfqtIndicativeQuotesAsync(
            nativeOrders[0].makerAssetData,
            nativeOrders[0].takerAssetData,
            MarketOperation.Sell,
            takerAmount,
            _opts,
        );
+
+        const balancerPromise = this._sampler.executeAsync(


Seems like balancer shouldn't belong to the sampler anymore?

moodlezoup force-pushed the feature/balancer-bridge branch 2 times, most recently from 8948fb8 to 61d4d0b Compare July 1, 2020 08:09

hysz approved these changes Jul 1, 2020

View reviewed changes

moodlezoup force-pushed the feature/balancer-bridge branch from e848669 to f93cb28 Compare July 1, 2020 21:42

moodlezoup requested a review from xianny July 1, 2020 21:58

xianny reviewed Jul 1, 2020

View reviewed changes

packages/asset-swapper/src/utils/market_operation_utils/curve_utils.ts Outdated Show resolved Hide resolved

xianny reviewed Jul 1, 2020

View reviewed changes

dekz reviewed Jul 2, 2020

View reviewed changes

moodlezoup force-pushed the feature/balancer-bridge branch from 8316c01 to ce6b665 Compare July 2, 2020 01:02

dorothy-zbornak reviewed Jul 2, 2020

View reviewed changes

moodlezoup force-pushed the feature/balancer-bridge branch 2 times, most recently from 9272410 to 181e67d Compare July 2, 2020 06:45

xianny force-pushed the feature/balancer-bridge branch 2 times, most recently from fb7fa55 to 0b0a52d Compare July 2, 2020 22:29

moodlezoup force-pushed the feature/balancer-bridge branch 2 times, most recently from 160b52b to 818fac1 Compare July 7, 2020 03:22

moodlezoup marked this pull request as ready for review July 7, 2020 19:35

moodlezoup requested review from abandeali1, BMillman19, dave4506 and fragosti as code owners July 7, 2020 19:35

dorothy-zbornak reviewed Jul 8, 2020

View reviewed changes

dekz reviewed Jul 8, 2020

View reviewed changes

moodlezoup force-pushed the feature/balancer-bridge branch 3 times, most recently from 3b63749 to a2fa30f Compare July 9, 2020 21:54

dorothy-zbornak reviewed Jul 10, 2020

View reviewed changes

moodlezoup force-pushed the feature/balancer-bridge branch from dbe595c to 529905e Compare July 10, 2020 17:10

moodlezoup force-pushed the feature/balancer-bridge branch from e2eb3a1 to 04839d3 Compare July 11, 2020 01:46

moodlezoup and others added 23 commits July 14, 2020 13:59

Add BalancerBridge and Sampler functions

f1a3a5b

Update sampler artifacts/wrappers

988696f

Add Balancer support to AssetSwapper + related refactoring

67bfaf5

Make use of GraphQL instead of sampler

dc1509c

"fix" build and add mainnet BalancerBridge tests

735648e

address some comments

c3b688a

add balancer cache and fix DexSampler tests

93726aa

lint

61b7918

wip: tests for balancer sampler ops

579564a

Fix market operation utils test

18c445f

balancer unit tests

81f4c8d

Return a buy quote of 0 if the buy amount exceeds the Balancer pool's…

2bec7e4

… balance

Dynamic fee estimation

732ece0

Update contract addresses, export BalancerBridge wrapper

9e36577

Update changelogs

7276527

Fix bugs discovered via simbot

8687508

Fix issues in balancer_utils

c80180c

override BigNumber.config in configured_bignumber.ts

3dee1d8

Special case Balancer subops in too

3b08137

Address some more comments

558ecbb

Address Balancer performance issue

7d60ee3

Performance improvements

fd968a3

Address comment

b4bee37

moodlezoup force-pushed the feature/balancer-bridge branch from 77306e2 to b4bee37 Compare July 14, 2020 21:00

Fix tests

ea3a0ec

dorothy-zbornak approved these changes Jul 14, 2020

View reviewed changes

dekz approved these changes Jul 15, 2020

View reviewed changes

xianny merged commit ff9c924 into development Jul 15, 2020

xianny deleted the feature/balancer-bridge branch July 15, 2020 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BalancerBridge #2613

BalancerBridge #2613

moodlezoup commented Jun 26, 2020 •

edited

Loading

hysz left a comment

xianny left a comment

dekz Jul 2, 2020

dekz Jul 2, 2020

dekz Jul 2, 2020

dorothy-zbornak left a comment

dorothy-zbornak Jul 2, 2020

dorothy-zbornak Jul 2, 2020

dorothy-zbornak left a comment

dorothy-zbornak Jul 8, 2020

dorothy-zbornak Jul 8, 2020

moodlezoup Jul 8, 2020

dorothy-zbornak Jul 8, 2020

dorothy-zbornak Jul 8, 2020

dekz left a comment •

edited

Loading

dekz Jul 8, 2020

moodlezoup Jul 8, 2020

dekz Jul 8, 2020

dekz Jul 8, 2020

dekz Jul 8, 2020 •

edited

Loading

moodlezoup Jul 8, 2020

dekz Jul 8, 2020

moodlezoup Jul 8, 2020 •

edited

Loading

dekz Jul 8, 2020

dorothy-zbornak Jul 10, 2020

dorothy-zbornak Jul 10, 2020

moodlezoup Jul 10, 2020

dorothy-zbornak Jul 10, 2020

dorothy-zbornak left a comment

dorothy-zbornak Jul 14, 2020

dorothy-zbornak Jul 14, 2020

	const minTimestamp = new Date().getTime() - this.cacheExpiryMs;
	const minTimestamp = Date.now() - this.cacheExpiryMs;

	const timestamp = new Date().getTime();
	const timestamp = Date.now();

	const key = JSON.stringify([takerToken, makerToken].sort());
	const key = JSON.stringify([takerToken, makerToken]);

		const samplerOps = subOps.filter(op => op.source !== ERC20BridgeSource.Balancer);
		const nonSamplerOps = subOps.filter(op => op.source === ERC20BridgeSource.Balancer);

BalancerBridge #2613

BalancerBridge #2613

Conversation

moodlezoup commented Jun 26, 2020 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

hysz left a comment

Choose a reason for hiding this comment

xianny 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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak 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

dekz left a comment • 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

dekz Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodlezoup Jul 8, 2020 • 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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodlezoup commented Jun 26, 2020 •

edited

Loading

dekz left a comment •

edited

Loading

dekz Jul 8, 2020 •

edited

Loading

moodlezoup Jul 8, 2020 •

edited

Loading