Eth2DaiBridge #2221

dorothy-zbornak · 2019-09-28T05:38:18Z

Description

Here's our first ever ERC20BridgeProxy (#2220) bridge: Eth2DaiBridge!.

Overview

This bridge contract is designed to be the makerAddress in all transactions, so:

Taker tokens get transferred to this contract.
ERC20BridgeProxy calls this contract, which:
1. Tries to sell its entire WETH or DAI balance to Eth2Dai.
2. Transfers everything it bought to the taker.

Because this contract should be the makerAddress of the order, it must sign for itself using the Wallet signature type. It implements the isValidSignature() callback but always succeeds.

Testing instructions

There are a whole 8 tests for this package. There really isn't much to unit test since the balance checks are enforced by the ERC20BridgeProxy and we just rely on Eth2Dai to do the actual conversion.

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.

Relates to 0xProject/ZEIPs/issues/47

coveralls · 2019-09-28T05:47:40Z

Coverage remained the same at 75.286% when pulling 65e1d0e on feat/erc20-bridge/eth2dai into 6da48be on feat/erc20-bridge/hello-world.

abandeali1

What are your thoughts on creating a new integrations package and moving this there? This can be used for all external integrations, and also provides a place for us to write end-to-end integration tests for the Exchange/Staking/Forwarder/Coordinator. CC @jalextowle

abandeali1 · 2019-09-30T17:18:48Z

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

+    ERC20Bridge,
+    IWallet
+{
+    bytes4 private constant LEGACY_WALLET_MAGIC_VALUE = 0xb0671381;


Maybe we should just include this in IWallet.sol and inherit that?

interfaces can't have/expose constants though, right?

Abstract contracts can though , which is what we already use for most interfaces

nvm, IWallet is actually a contract.

I ended up just making a copy in asset-proxy since it would be a circular dep.

abandeali1 · 2019-09-30T17:20:20Z

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

+    bytes4 private constant LEGACY_WALLET_MAGIC_VALUE = 0xb0671381;
+    /* Mainnet addresses */
+    address constant public ETH2DAI_ADDRESS = 0x39755357759cE0d7f32dC8dC45414CCa409AE24e;
+    address constant public WETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;


Ideally we don't have to hard code any token addresses. It looks like Eth2Dai supports other token pairs in theory, and those should be able to be used as well. The fromToken can probably be encoded in the bridgeData.

abandeali1 · 2019-09-30T17:23:05Z

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

+            amount
+        );
+        // Transfer the converted `toToken`s to `to`.
+        toToken.transfer(to, boughtAmount);


We should probably check the return value here, just in case a token isn't 100% ERC20 compliant.

jalextowle

LGTM

…dge`.

…n in `IEth2Dai`.

…lector.

…y tokens. `@0x/contracts-asset-proxy`: Support non-conformant tokens in Eth2DaiBridge

abandeali1 · 2019-10-01T22:29:42Z

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

+    )
+        private
+    {
+        mapping (address => bool) storage spenderHasAllowance = _hasAllowance[spender];


What happens when the allowance decreases to less than the required amount, but this is still set to true?

I think that this check actually just ends up being an extra expense most of the time. I'd prefer just calling IERC20Token(tokenAddress).approve(spender, uint256(-1)) every single time. If an allowance is already set, then this won't cost much extra gas at all. This also allows us to remove the _hasAllowance mapping altogether.

I'm OK with this.

abandeali1 · 2019-10-01T22:35:35Z

contracts/asset-proxy/contracts/src/interfaces/IWallet.sol

+
+contract IWallet {
+
+    bytes4 internal constant LEGACY_WALLET_MAGIC_VALUE = 0xb0671381;


I guess we can't import this due to circular dependencies?

Yep. The alternative is to have exchange import this file from the asset-proxy package, but that just feels wrong.

We should probably create a task for moving this to exchange-libs, but doesn't have to be in this PR.

abandeali1

One more small nit, then LGTM!

abandeali1 · 2019-10-02T02:10:22Z

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

+
+        IEth2Dai exchange = _getEth2DaiContract();
+        // Grant an allowance to the exchange to spend `fromTokenAddress` token.
+        _grantAllowanceForToken(address(exchange), fromTokenAddress);


Nit: we can probably just remove this function. It's pretty simple, and we also use IERC20Token(fromTokenAddress).balanceOf below.

…ridge`.

dorothy-zbornak marked this pull request as ready for review September 28, 2019 06:05

dorothy-zbornak requested review from abandeali1 and hysz as code owners September 28, 2019 06:05

dorothy-zbornak changed the title ~~[WIP] Eth2DaiBridge~~ Eth2DaiBridge Sep 28, 2019

dorothy-zbornak force-pushed the feat/erc20-bridge/eth2dai branch from b1098ca to 6355a8a Compare September 30, 2019 01:59

abandeali1 suggested changes Sep 30, 2019

View reviewed changes

jalextowle approved these changes Sep 30, 2019

View reviewed changes

dorothy-zbornak force-pushed the feat/erc20-bridge/eth2dai branch from 6355a8a to 729a8b5 Compare October 1, 2019 00:13

dorothy-zbornak added 11 commits September 30, 2019 17:29

@0x/contracts-asset-proxy: Add Eth2DaiBridge and tests.

b9a1fb4

@0x/contracts-asset-proxy: Add signature validation.

7d344b3

@0x/contracts-asset-proxy: Remove only modifier on tests.

87ee8f2

@0x/contracts-asset-proxy: Add some more unit tests for `Eth2DaiBri…

59625ae

…dge`.

@0x/contracts-asset-proxy: omg these insidious only modifiers.

3ba04ef

@0x/contracts-asset-proxy: Rebase and add extra comments.

6a9fe16

@0x/contracts-asset-proxy: Add comments and update naming conventio…

2dab5f0

…n in `IEth2Dai`.

@0x/contracts-asset-proxy: Rebase and update tests for new proxy se…

d4b3eca

…lector.

@0x/contracts-asset-proxy: Switch Eth2DaiBridge to support arbitrar…

2affd98

…y tokens. `@0x/contracts-asset-proxy`: Support non-conformant tokens in Eth2DaiBridge

@0x/contracts-asset-proxy: Rebase and use @0x/types.AssetProxyId.

af3ab05

@0x/contracts-asset-proxy: Rebase and use IERC20Bridge

8db3815

dorothy-zbornak force-pushed the feat/erc20-bridge/eth2dai branch from 729a8b5 to 8db3815 Compare October 1, 2019 00:43

mintcloud assigned abandeali1 and jalextowle Oct 1, 2019

abandeali1 suggested changes Oct 1, 2019

View reviewed changes

dorothy-zbornak mentioned this pull request Oct 2, 2019

UniswapBridge #2233

Merged

4 tasks

@0x/contracts-asset-proxy: Set allowance every time.

5cfa70a

abandeali1 approved these changes Oct 2, 2019

View reviewed changes

@0x/contracts-asset-proxy: Inline _grantAllowances() in `Eth2DaiB…

65e1d0e

…ridge`.

dorothy-zbornak merged commit 96aa149 into feat/erc20-bridge/hello-world Oct 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth2DaiBridge #2221

Eth2DaiBridge #2221

dorothy-zbornak commented Sep 28, 2019 •

edited

Loading

coveralls commented Sep 28, 2019 •

edited

Loading

abandeali1 left a comment

abandeali1 Sep 30, 2019

dorothy-zbornak Sep 30, 2019

jalextowle Sep 30, 2019

dorothy-zbornak Sep 30, 2019

dorothy-zbornak Sep 30, 2019

abandeali1 Sep 30, 2019 •

edited

Loading

abandeali1 Sep 30, 2019

jalextowle left a comment

abandeali1 Oct 1, 2019

dorothy-zbornak Oct 2, 2019

abandeali1 Oct 1, 2019

dorothy-zbornak Oct 2, 2019

abandeali1 Oct 2, 2019

abandeali1 left a comment

abandeali1 Oct 2, 2019


		contract IWallet {

		bytes4 internal constant LEGACY_WALLET_MAGIC_VALUE = 0xb0671381;

Eth2DaiBridge #2221

Eth2DaiBridge #2221

Conversation

dorothy-zbornak commented Sep 28, 2019 • edited Loading

Description

Overview

Testing instructions

Types of changes

Checklist:

coveralls commented Sep 28, 2019 • edited Loading

abandeali1 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

abandeali1 Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalextowle 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

abandeali1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak commented Sep 28, 2019 •

edited

Loading

coveralls commented Sep 28, 2019 •

edited

Loading

abandeali1 Sep 30, 2019 •

edited

Loading