Add MultiBridge support to AssetSwapper #2593

moodlezoup · 2020-06-02T22:13:12Z

Description

Adds a function to the Sampler contract to sample from MultiBridge
Adds corresponding sampler operation to AS
Some other MB-specific logic in AS
Regenerates contract artifacts/wrappers

Testing instructions

Types of changes

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.

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

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

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

dorothy-zbornak · 2020-06-04T13:53:20Z

packages/asset-swapper/test/market_operation_utils_test.ts

@@ -648,14 +651,14 @@ describe('MarketOperationUtils tests', () => {
                    { excludedSources: SELL_SOURCES, numSamples: 4, bridgeSlippage: 0, shouldBatchBridgeOrders: false },
                );
                expect(result.length).to.eql(1);
-                expect(result[0].makerAddress).to.eql(liquidityProviderAddress);
+                expect(result[0].makerAddress).to.eql(DEX_FORWARDER_BRIDGE_ADDRESS);


Why does this happen? shouldBatchBridgeOrders is false.

ah yes that would explain the failing test, idk how this slipped in

dorothy-zbornak · 2020-06-04T14:19:18Z

Can you explain to me the reasoning behind the MB registry? Will there be different MB contracts for different pairs?

moodlezoup · 2020-06-05T18:20:22Z

Can you explain to me the reasoning behind the MB registry? Will there be different MB contracts for different pairs?

probably not –– I decided on the registry layer so we could swap out MB implementations without disrupting the API. Also considered using a proxy pattern, but decided against it for PLP-related security reasons

dorothy-zbornak · 2020-06-05T21:28:31Z

Can you explain to me the reasoning behind the MB registry? Will there be different MB contracts for different pairs?

probably not –– I decided on the registry layer so we could swap out MB implementations without disrupting the API. Also considered using a proxy pattern, but decided against it for PLP-related security reasons

I can appreciate seamless updates, but I wonder if it's easier to update AS + API than hunt down the person with the MB registry private key. I guess it could be worth it if MB is getting frequent updates. Not a blocker, either way.

dorothy-zbornak · 2020-06-05T21:36:02Z

Can you explain to me the reasoning behind the MB registry? Will there be different MB contracts for different pairs?

probably not –– I decided on the registry layer so we could swap out MB implementations without disrupting the API. Also considered using a proxy pattern, but decided against it for PLP-related security reasons

I can appreciate seamless updates, but I wonder if it's easier to update AS + API than hunt down the person with the MB registry private key. I guess it could be worth it if MB is getting frequent updates. Not a blocker, either way.

Actually now I'm slightly leaning against the registry. I think there might be value in AS/API opting into a version of MB. This gives us the ability to easily rollback to an older MB if necessary and also makes it easier to AB test one MB integration vs another.

moodlezoup · 2020-06-05T22:17:42Z

yeah that makes sense. will update

dekz

I agree that the registry seems unnecessary, unless theres the case where there's many of them and they're different.

Down to keep passing in the address as a param (rather than hard code like Kyber/Uniswap) to save us re-deploying the Sampler contract with an updated MultiBridge address.

dekz · 2020-06-08T03:13:57Z

packages/contract-addresses/addresses.json

@@ -20,7 +20,7 @@
        "devUtils": "0x74134cf88b21383713e096a5ecf59e297dc7f547",
        "erc20BridgeProxy": "0x8ed95d1746bf1e4dab58d8ed4724f1ef95b20db0",
        "uniswapBridge": "0x36691c4f426eb8f42f150ebde43069a31cb080ad",
-        "erc20BridgeSampler": "0xc0154b14cc60a6661171fdc817f759429a57e184",
+        "erc20BridgeSampler": "0xd7223e59a11bbeb663c77625f06963ff42fdf97d",


Can we get this source verified or redeploy if the artifact is lost prior to merge?

I'll redeploy, verify, and update before merging

dekz · 2020-06-10T07:18:50Z

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

@@ -75,35 +77,37 @@ export class MarketOperationUtils {
        }
        const _opts = { ...DEFAULT_GET_MARKET_ORDERS_OPTS, ...opts };
        const [makerToken, takerToken] = getNativeOrderTokens(nativeOrders[0]);
+        const optionalSources = (this._liquidityProviderRegistry ? [ERC20BridgeSource.LiquidityProvider] : []).concat(
+            this._multiBridge === NULL_ADDRESS ? [ERC20BridgeSource.MultiBridge] : [],


should this be

Suggested change

this._multiBridge === NULL_ADDRESS ? [ERC20BridgeSource.MultiBridge] : [],

this._multiBridge !== NULL_ADDRESS ? [ERC20BridgeSource.MultiBridge] : [],

ah shit 🙈

dekz · 2020-06-10T07:23:57Z

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


    constructor(
        private readonly _sampler: DexOrderSampler,
        private readonly contractAddresses: ContractAddresses,
        private readonly _orderDomain: OrderDomain,
-        private readonly _liquidityProviderRegistry: string = NULL_ADDRESS,
+        private readonly _liquidityProviderRegistry?: string,


This change does seem a little inconsistent with the following multiBridge. What are your thoughts on this change?

hm this must be a relic of some older changes; this branch is pretty old. good catch, will fix

dorothy-zbornak

ya looks good!

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

xianny · 2020-06-11T20:08:58Z

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

-    if (source === ERC20BridgeSource.Kyber) {
-        return FillFlags.Kyber;
+    switch (source) {
+        case ERC20BridgeSource.Uniswap:


Are we intentionally excluding ERC20BridgeSource.UniswapV2 and ERC20BridgeSource.UniswapV2Eth here?

yes, MultiBridge is not yet integrated with v2

moodlezoup force-pushed the feature/multibridge/asset-swapper branch 2 times, most recently from a36c407 to 7579451 Compare June 3, 2020 18:22

moodlezoup changed the title ~~Feature/multibridge/asset swapper~~ Add MultiBridge support to AssetSwapper Jun 3, 2020

moodlezoup marked this pull request as ready for review June 3, 2020 18:30

moodlezoup requested review from abandeali1, BMillman19, dave4506, fragosti and hysz as code owners June 3, 2020 18:30

moodlezoup requested review from dorothy-zbornak and dekz and removed request for BMillman19, fragosti, hysz, dave4506 and abandeali1 June 3, 2020 18:30

moodlezoup force-pushed the feature/multibridge/asset-swapper branch from 7579451 to d3cbdf1 Compare June 3, 2020 18:47

dorothy-zbornak reviewed Jun 4, 2020

View reviewed changes

dekz reviewed Jun 8, 2020

View reviewed changes

moodlezoup force-pushed the feature/multibridge/asset-swapper branch 3 times, most recently from dbba8b0 to 213be97 Compare June 10, 2020 02:29

dekz reviewed Jun 10, 2020

View reviewed changes

moodlezoup force-pushed the feature/multibridge/asset-swapper branch from 213be97 to 1b5746b Compare June 10, 2020 18:52

moodlezoup requested a review from dekz June 10, 2020 18:54

moodlezoup requested a review from dorothy-zbornak June 10, 2020 18:54

dorothy-zbornak approved these changes Jun 11, 2020

View reviewed changes

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

moodlezoup force-pushed the feature/multibridge/asset-swapper branch from 1b5746b to ce5ec62 Compare June 11, 2020 18:10

MultiBridge support in AssetSwapper

44262bf

moodlezoup force-pushed the feature/multibridge/asset-swapper branch 3 times, most recently from fc482ef to 8609514 Compare June 11, 2020 19:24

Update sampler artifacts/wrappers

cd14a45

moodlezoup force-pushed the feature/multibridge/asset-swapper branch from 8609514 to cd14a45 Compare June 11, 2020 19:48

xianny reviewed Jun 11, 2020

View reviewed changes

xianny mentioned this pull request Jun 11, 2020

Update wrappers and artifacts for ERC20BridgeSampler #2600

Closed

4 tasks

moodlezoup merged commit e936c7c into development Jun 11, 2020

moodlezoup deleted the feature/multibridge/asset-swapper branch June 11, 2020 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MultiBridge support to AssetSwapper #2593

Add MultiBridge support to AssetSwapper #2593

moodlezoup commented Jun 2, 2020 •

edited

Loading

dorothy-zbornak Jun 4, 2020

moodlezoup Jun 5, 2020

dorothy-zbornak commented Jun 4, 2020

moodlezoup commented Jun 5, 2020 •

edited

Loading

dorothy-zbornak commented Jun 5, 2020

dorothy-zbornak commented Jun 5, 2020

moodlezoup commented Jun 5, 2020

dekz left a comment

dekz Jun 8, 2020

moodlezoup Jun 9, 2020

dekz Jun 10, 2020

moodlezoup Jun 10, 2020

dekz Jun 10, 2020

moodlezoup Jun 10, 2020

dorothy-zbornak left a comment

xianny Jun 11, 2020

moodlezoup Jun 11, 2020

	this._multiBridge === NULL_ADDRESS ? [ERC20BridgeSource.MultiBridge] : [],
	this._multiBridge !== NULL_ADDRESS ? [ERC20BridgeSource.MultiBridge] : [],

Add MultiBridge support to AssetSwapper #2593

Add MultiBridge support to AssetSwapper #2593

Conversation

moodlezoup commented Jun 2, 2020 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak commented Jun 4, 2020

moodlezoup commented Jun 5, 2020 • edited Loading

dorothy-zbornak commented Jun 5, 2020

dorothy-zbornak commented Jun 5, 2020

moodlezoup commented Jun 5, 2020

dekz 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

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 2, 2020 •

edited

Loading

moodlezoup commented Jun 5, 2020 •

edited

Loading