Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

ZeroEx: ERC20 Transformers #2576

Merged

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented May 5, 2020

Description

tenor

Here are the first 3 transformers for use in the TransformERC20 feature debuted in #2545:

  • WethTransformer
    • Wraps ETH or unwraps WETH.
  • PayTakerTransformer
    • Transfer tokens/ETH to the taker.
  • FillQuoteTransformer
    • Fills a buy or sell quote.

So for an ETH -> DAI quote, we might autobots assemble:

  1. WethTransformer({token: ETH})
  2. FillQuoteTransformer({sellToken: WETH, buyToken: DAI})
  3. PayTakerTransformer({tokens: [DAI, ETH]}) (ETH to refund protocol fees)

Or for DAI -> ETH:

  1. FillQuoteTransformer({sellToken: DAI, buyToken: WETH})
  2. WethTransformer({token: WETH})
  3. PayTakerTransformer({tokens: [ETH]})

Or for DAI -> USDC

  1. FillQuoteTransformer({sellToken: DAI, buyToken: USDC})
  2. PayTakerTransformer({tokens: [USDC, ETH]}) (ETH to refund protocol fees)

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 dorothy-zbornak changed the base branch from development to feat/contracts-zero-ex/0x-api-market-fns May 5, 2020 07:25
@dorothy-zbornak dorothy-zbornak changed the title [WIP] ERC20 Transformers [WIP] ZeroEx: ERC20 Transformers May 5, 2020
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from 4cba718 to 1fc7d3f Compare May 5, 2020 07:33
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch 2 times, most recently from 3c1e4c1 to 0dfca8e Compare May 5, 2020 07:37
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from 1fc7d3f to a3ba9c6 Compare May 5, 2020 07:39
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch from 0dfca8e to 45d8753 Compare May 5, 2020 07:41
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from a3ba9c6 to 64af981 Compare May 5, 2020 15:15
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch from 45d8753 to ad3387e Compare May 5, 2020 15:16
@coveralls
Copy link

coveralls commented May 5, 2020

Coverage Status

Coverage decreased (-0.2%) to 79.512% when pulling 8bb8c3d on feat/contracts-zero-ex/0x-api-erc20-transformers into 2fce332 on development.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch 3 times, most recently from 94d0da9 to c51b9e5 Compare May 8, 2020 16:28
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 6 times, most recently from ad91546 to 0afcc07 Compare May 14, 2020 06:33
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch 2 times, most recently from 29e21f0 to 88bd8d0 Compare May 14, 2020 15:06
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from 0afcc07 to b4c5081 Compare May 14, 2020 15:43
@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch 4 times, most recently from 36c8393 to 0f4f4ed Compare May 15, 2020 05:27
@dorothy-zbornak dorothy-zbornak marked this pull request as ready for review May 15, 2020 05:31
/// @dev Destruct this contract. Only callable by the deployer and will not
/// succeed in the context of a delegatecall (from another contract).
/// @param ethRecipient The recipient of ETH held in this contract.
function die(address payable ethRecipient)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be able to murder transformers since there's no way to un-register them.

uint256 takerTokenFillAmount = sellAmount;

if (order.takerFee != 0) {
if (takerFeeToken == makerToken) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on using the sort of "loose" asset equality we're using in the Forwarder? see MixinExchangeWrapper::_areUnderlyingAssetsEqual

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really necessary for our current bridges, I suppose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can just quickly deploy a new one when we have more interesting orders coming out of the API.



/// @dev Interface to the V3 Exchange.
interface IExchange {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jw what's the etymology of this "vendor" directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vendor is convention for third party stuff. Right now it's just the exchange but in the future I would put uniswap, oasis, kyber interfaces in there. https://softwareengineering.stackexchange.com/questions/123305/what-is-the-difference-between-the-lib-and-vendor-folders

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking solid!

/// @dev The ERC20Proxy address.
address public immutable erc20Proxy;

using LibERC20TokenV06 for IERC20TokenV06;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - can we place these at the top of contract for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk I kind of like having them down before the first function since that's when the syntactic sugar can actually get used. The rest of the code base is like this as well so I'd have to touch nearly every other contract. We can do a separate re-styling PR prior to deployment if we agree it's better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yeah not a blocker, but we should probably put these at the top since they apply to the entire contract.

if (amount == uint256(-1)) {
amount = data.tokens[i].getTokenBalanceOf(address(this));
}
if (amount != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should error out when amount == 0 🤔. Is there a use case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. The PayTakerTransformer is also used to refund unspent protocol fees, which could end up being 0 if they were all used.

@dorothy-zbornak dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-erc20-transformers branch from 12b4eaf to ecfbd62 Compare May 28, 2020 15:57
Copy link
Contributor

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excited to see these roll out! (🥁 tss)

// The token to wrap/unwrap. Must be either ETH or WETH.
IERC20TokenV06 token;
// Amount of `token` to wrap or unwrap.
// `uint(-1)` will unwrap the entire balance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try to make these API's as explicit as possible. For example, we could standardize that if a transformer has more than one action we'll use an Enum to differentiate:

enum Action {
    UNWRAP,
    WRAP
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, is 1 or 0 any more explicit than passing in 2 distinct addresses?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking of like a separate transformer for wrapping and unwrapping? That's probably fine too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw I think the confusion just arises here because we're overloading the uint(-1). I think this is generally used to mean like the whole balance, but here we also use it to flip the operation. I don't feel too strongly either way, but worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use uint256(-1) to flip the operation, we specify either the WETH address or the ETH address (0xeee...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{token: 0xeeee..., amount: uint256(-1): Wrap all ETH.
{token: WETH_ADDRESS, amount: uint256(-1): Unwrap all WETH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, that makes more sense. +1

data.fillAmount.safeSub(soldAmount).min256(
data.maxOrderFillAmounts.length > i
? data.maxOrderFillAmounts[i]
: uint256(-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - use a constant here to help readability.

data.fillAmount
).rrevert();
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an automatic check when decoding that enforces data.side is Side.Sell or Side.Buy? If not we should probably do ...else if (data.side == Side.Buy) {..} else {revert()}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the abi.decode() will do a range check on the enum value.

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!! I think just a few minor thoughts and nits then good to merge! (~‾▿‾)~

@dorothy-zbornak dorothy-zbornak merged commit 0c6e05d into development Jun 3, 2020
@dorothy-zbornak dorothy-zbornak deleted the feat/contracts-zero-ex/0x-api-erc20-transformers branch June 3, 2020 18:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants