-
Notifications
You must be signed in to change notification settings - Fork 465
ZeroEx: ERC20 Transformers #2576
ZeroEx: ERC20 Transformers #2576
Conversation
4cba718
to
1fc7d3f
Compare
3c1e4c1
to
0dfca8e
Compare
1fc7d3f
to
a3ba9c6
Compare
0dfca8e
to
45d8753
Compare
a3ba9c6
to
64af981
Compare
45d8753
to
ad3387e
Compare
94d0da9
to
c51b9e5
Compare
ad91546
to
0afcc07
Compare
29e21f0
to
88bd8d0
Compare
0afcc07
to
b4c5081
Compare
36c8393
to
0f4f4ed
Compare
/// @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) |
There was a problem hiding this comment.
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.
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
uint256 takerTokenFillAmount = sellAmount; | ||
|
||
if (order.takerFee != 0) { | ||
if (takerFeeToken == makerToken) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
contracts/zero-ex/contracts/src/transformers/PayTakerTransformer.sol
Outdated
Show resolved
Hide resolved
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
contracts/zero-ex/contracts/src/transformers/WethTransformer.sol
Outdated
Show resolved
Hide resolved
|
||
|
||
/// @dev Interface to the V3 Exchange. | ||
interface IExchange { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking solid!
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
/// @dev The ERC20Proxy address. | ||
address public immutable erc20Proxy; | ||
|
||
using LibERC20TokenV06 for IERC20TokenV06; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
contracts/zero-ex/contracts/src/transformers/FillQuoteTransformer.sol
Outdated
Show resolved
Hide resolved
if (amount == uint256(-1)) { | ||
amount = data.tokens[i].getTokenBalanceOf(address(this)); | ||
} | ||
if (amount != 0) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…inct contract type and rename `getTokenSpenderPuppet()` to `getAllowanceTarget()`
…storage buckets.
12b4eaf
to
ecfbd62
Compare
There was a problem hiding this 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. |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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()}
There was a problem hiding this comment.
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.
There was a problem hiding this 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! (~‾▿‾)~
Description
Here are the first 3 transformers for use in the
TransformERC20
feature debuted in #2545:So for an ETH -> DAI quote, we might autobots assemble:
WethTransformer({token: ETH})
FillQuoteTransformer({sellToken: WETH, buyToken: DAI})
PayTakerTransformer({tokens: [DAI, ETH]})
(ETH to refund protocol fees)Or for DAI -> ETH:
FillQuoteTransformer({sellToken: DAI, buyToken: WETH})
WethTransformer({token: WETH})
PayTakerTransformer({tokens: [ETH]})
Or for DAI -> USDC
FillQuoteTransformer({sellToken: DAI, buyToken: USDC})
PayTakerTransformer({tokens: [USDC, ETH]})
(ETH to refund protocol fees)Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.