ERC20BridgeProxy

[dorothy-zbornak]

Description

Say hello to the new ERC20BridgeProxy!

Overview

The ERC20BridgeProxy essentially does just two things:

1. Call withdrawTo() on an instance of the IERC20Bridge contract.
2. Ensure that the token balance of to has increased by amount after the call.

It is up to the IERC20Bridge contract to actually perform the transfer. If the bridge contract needs the taker funds in order to complete the transfer (like for Eth2Dai/Uniswap bridges), the order should be constructed with maker = IERC20Bridge.

AssetData

abi.encodeWithSelector(
    // bytes4(keccak256("ERC20BridgeProxy(address,address,bytes)"))
    ERC20BridgeProxy(0).PROXY_ID,
    // The address of the ERC20 token that will be transferred from `from` to `to`
    address tokenAddress,
    // The address of an instance of the `IERC20Bridge` contract that will handle
    // the transfer.
    address bridgeAddress,
    // Arbitrary data needed by the bridge contract.
    bytes bridgeData
)

IERC20Bridge

A valid bridge contract only has to export one function:

/// @dev Transfers `amount` of the ERC20 `tokenAddress` from `from` to `to`.
/// @param tokenAddress The address of the ERC20 token to transfer.
/// @param from Address to transfer asset from.
/// @param to Address to transfer asset to.
/// @param amount Amount of asset to transfer.
/// @param bridgeData Arbitrary asset data needed by the bridge contract.
/// @return success The magic bytes `0xb5d40d78` if successful.
function transfer(
    address tokenAddress,
    address from,
    address to,
    uint256 amount,
    bytes calldata bridgeData
)
    external
    returns (bytes4 success);

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.

Addresses 0xProject/ZEIPs/issues/47

[coveralls]

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

[abandeali1]

So excited for this!

[abandeali1]

How was this derived?

[dorothy-zbornak]

crypto.randomBytes(4)

[abandeali1]

Probably better to add some meaning to them. Maybe just reuse the PROXY_ID?

[abandeali1]

Some nits about the signature of this function:

transferFrom is more descriptive than transfer. It's somewhat awkward that this would be the 3rd different transferFrom used in the proxies, so maybe there is a better name... but I don't love transfer.

I also think bridgeData should be the last parameter. It is optional and probably won't be used in all bridge contracts. Making it the last parameter is consistent with all of the token standards that have a similar parameter in their own transfer functions.

[dorothy-zbornak]

I would like to avoid transferFrom because it makes it really hard to explain this pattern to people (transferFrom calls transferFrom calls transferFrom). Maybe convertFrom? exchangeFrom? bridgeFrom? swapFrom? settleFrom? I'm at a loss.

[abandeali1]

withdrawTo perhaps?

[abandeali1]

Nit: I feel like balanceOf(tokenAddress, to) >= balanceBefore.safeAdd(amount) is slightly more intuitive. Not a huge deal either way.

[abandeali1]

This can be made more efficient with:

(address tokenAddress) = abi.decode(
    assetData.sliceDestructive(4, assetData.length),
    (address)
);

[dorothy-zbornak]

Yeah, I was trying to sneak in some extra asset data validation this way, but I guess since it's not checking the selector, it's not even doing that very well.

[abandeali1]

This should probably use ERC20Bridge rather than ERC20BridgeProxy to be consistent with the other ids. We should also add an entry for this to IAssetData.

[jalextowle]

This is looking great! I just had a few nits on tests, and then this looks good to go

[jalextowle]

Maybe we should add this to AssetProxyId in packages/types/

[jalextowle]

Nit:

Suggested change

	it('succeeds if the bridge succeeds and balance increases', async () => {
	it('succeeds if the bridge succeeds and balance increases by `amount`', async () => {

[jalextowle]

Nit:

Suggested change

	it('fails if balance of `to` increases by `amount - 1`', async () => {
	it('fails if balance of `to` increases by less than `amount`', async () => {

For consistency with the above tests.

[jalextowle]

🙏

[jalextowle]

Nice!

[hysz]

looks great +1! Please hit http://merge.it (~‾▿‾)~