DexForwarderBridge #2525

dorothy-zbornak · 2020-03-18T18:04:49Z

Description

We can reduce swap protocol fees by unifying bridge orders under a single fill. We can take advantage of the fact that DEX bridges are permissionless. This PR introduces the DexForwarderBridge bridge, which essentially performs market fills over a sequence of bridge calls.

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.

dorothy-zbornak · 2020-03-26T05:57:31Z

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

+
+        for (uint256 i = 0; i < state.calls.length; ++i) {
+            // Stop if the we've sold all our input tokens.
+            if (state.totalInputTokenSold >= state.initialInputTokenBalance) {


I had a version where we also could stop based on output tokens bought (market buy), but ran into some issues:

we can't use amount because we underestimate this value in AS w/ bridgeSlippage

this contract can end up with a balance of input tokens, so would we refund those to to?

I'm not sure this is the same behavior you would get if not using the DFB (individual bridge orders)

Not sure any of this is even worth it.

yeah I think this is probably cleaner

coveralls · 2020-03-26T06:13:09Z

Coverage increased (+0.2%) to 79.739% when pulling c5a6e49 on feat/bridge/DexForwarderBridge into 277dbac on development.

moodlezoup

good stuff, just some typos/nits

moodlezoup · 2020-03-26T06:02:58Z

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

+            state.calls
+        ) = abi.decode(bridgeData, (address, BridgeCall[]));
+
+        state.initialInputTokenBalance = IERC20Token(state.inputToken).balanceOf(address(this));


might as well use LibERC20Token for balances

LibERC20Token.balanceOf() swallows reverts. Not my jam.

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

moodlezoup · 2020-03-26T06:32:54Z

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

+                call.outputTokenAmount
+            );
+
+            (bool didSucceed, ) = address(this)


Maybe we should decode the return data here and check that it equals BRIDGE_SUCCESS?

executeBridgeCall() doesn't return anything. It just succeeds or reverts.

moodlezoup · 2020-03-26T06:36:44Z

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

+
+        for (uint256 i = 0; i < state.calls.length; ++i) {
+            // Stop if the we've sold all our input tokens.
+            if (state.totalInputTokenSold >= state.initialInputTokenBalance) {


yeah I think this is probably cleaner

dekz · 2020-03-26T06:28:32Z

contracts/asset-proxy/src/dex_forwarder_bridge.ts

+export interface DexForwaderBridgeCall {
+    target: string;
+    inputTokenAmount: BigNumber;
+    outputTokenAmount: BigNumber;
+    bridgeData: string;
+}
+
+export interface DexForwaderBridgeData {
+    inputToken: string;
+    calls: DexForwaderBridgeCall[];


Suggested change

export interface DexForwaderBridgeCall {

target: string;

inputTokenAmount: BigNumber;

outputTokenAmount: BigNumber;

bridgeData: string;

}

export interface DexForwaderBridgeData {

inputToken: string;

calls: DexForwaderBridgeCall[];

export interface DexForwarderBridgeCall {

target: string;

inputTokenAmount: BigNumber;

outputTokenAmount: BigNumber;

bridgeData: string;

}

export interface DexForwarderBridgeData {

inputToken: string;

calls: DexForwarderBridgeCall[];

help me jesus

dekz · 2020-03-26T06:30:17Z

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

+        // Revert if we were not able to sell our entire input token balance.
+        require(
+            state.totalInputTokenSold >= state.initialInputTokenBalance,
+            "DexForwaderBridge/INCOMPLETE_FILL"


Suggested change

"DexForwaderBridge/INCOMPLETE_FILL"

"DexForwarderBridge/INCOMPLETE_FILL"

and a few more 😆

dekz · 2020-03-26T20:23:31Z

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

+
+            if (!didSucceed) {
+                // Log errors.
+                emit DexForwarderBridgeCallFailed(


Is this just for internal data gathering/insights?

AS makes heavy use of FillOrKill so the only case where it'd reasonably be emitted is when succeed with enough slippage/backup path?

Yeah, that's what I'm thinking. These won't be going through fillOrder() or the ERC20BridgeProxy so it's a less obvious path to detecting them. Maybe it's not worth it, though, since the bridge itself can revert if not enough input tokens are sold. We'd have to rely on traces for those situations (and the market fn failing).

Now I'm leaning towards nixing this event.

…act.

…CallFailed` event from `DexForwarderBridge`.

…l()` test.

`@0x/migrations`: Add `dexForwarderBridge` address field.

dorothy-zbornak force-pushed the feat/bridge/DexForwarderBridge branch 2 times, most recently from 75b8bf7 to ade380d Compare March 18, 2020 20:26

dorothy-zbornak force-pushed the feat/bridge/DexForwarderBridge branch 2 times, most recently from 5bf0ead to ab409f4 Compare March 26, 2020 05:48

dorothy-zbornak marked this pull request as ready for review March 26, 2020 05:48

dorothy-zbornak requested review from abandeali1 and hysz as code owners March 26, 2020 05:48

dorothy-zbornak commented Mar 26, 2020

View reviewed changes

dorothy-zbornak requested a review from dekz March 26, 2020 05:59

moodlezoup approved these changes Mar 26, 2020

View reviewed changes

dekz approved these changes Mar 26, 2020

View reviewed changes

dorothy-zbornak force-pushed the feat/bridge/DexForwarderBridge branch from 2e5bf37 to 02e47c7 Compare March 27, 2020 20:35

merklejerk and others added 6 commits March 27, 2020 16:39

@0x/contracts-asset-proxy: Create DexForwarderBridge bridge contr…

4bdaa48

…act.

@0x/contracts-asset-proxy: Address review comments.

338c358

@0x/contracts-asset-proxy: Fix typo in DFB tests.

f3c6f26

@0x/contracts-asset-proxy: Fix typos and remove `BridgeTransferFrom…

0ccbc7d

…CallFailed` event from `DexForwarderBridge`.

@0x/contracts-asset-proxy: Add `DexForwarderBridge.executeBridgeCal…

1f789bf

…l()` test.

@0x/contract-addresses: Add dexForwarderBridge address.

a509af2

`@0x/migrations`: Add `dexForwarderBridge` address field.

dorothy-zbornak force-pushed the feat/bridge/DexForwarderBridge branch from 02e47c7 to a509af2 Compare March 27, 2020 20:40

update ganache forwarder address

c5a6e49

dorothy-zbornak merged commit bf00e67 into development Mar 27, 2020

dorothy-zbornak deleted the feat/bridge/DexForwarderBridge branch March 27, 2020 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DexForwarderBridge #2525

DexForwarderBridge #2525

dorothy-zbornak commented Mar 18, 2020 •

edited

Loading

dorothy-zbornak Mar 26, 2020 •

edited

Loading

moodlezoup Mar 26, 2020

coveralls commented Mar 26, 2020 •

edited

Loading

moodlezoup left a comment

moodlezoup Mar 26, 2020

dorothy-zbornak Mar 26, 2020

moodlezoup Mar 26, 2020

dorothy-zbornak Mar 26, 2020 •

edited

Loading

moodlezoup Mar 26, 2020

dekz Mar 26, 2020

dorothy-zbornak Mar 26, 2020

dekz Mar 26, 2020

dekz Mar 26, 2020

dekz Mar 26, 2020

dorothy-zbornak Mar 26, 2020

	"DexForwaderBridge/INCOMPLETE_FILL"
	"DexForwarderBridge/INCOMPLETE_FILL"

DexForwarderBridge #2525

DexForwarderBridge #2525

Conversation

dorothy-zbornak commented Mar 18, 2020 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

dorothy-zbornak Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 26, 2020 • edited Loading

moodlezoup 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 Mar 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak commented Mar 18, 2020 •

edited

Loading

dorothy-zbornak Mar 26, 2020 •

edited

Loading

coveralls commented Mar 26, 2020 •

edited

Loading

dorothy-zbornak Mar 26, 2020 •

edited

Loading