ZeroEx: TransformERC20, TokenSpender #2545

dorothy-zbornak · 2020-04-09T17:23:22Z

Description

This adds the TransformERC20 and TokenSpender feature to the ZeroEx contract. Please refer to the internal spec for background on these features.

This PR does not include the transformers themselves. Those are in #2576.

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.

coveralls · 2020-04-29T20:21:27Z

Coverage decreased (-0.3%) to 79.361% when pulling 0c83752 on feat/contracts-zero-ex/0x-api-market-fns into d9e13d6 on development.

dorothy-zbornak · 2020-05-05T07:29:27Z

contracts/erc20/contracts/src/v06/LibERC20TokenV06.sol

+import "./IERC20TokenV06.sol";
+
+
+library LibERC20TokenV06 {


This library has significant changes from the original (0.5) version.

dorothy-zbornak · 2020-05-05T07:34:44Z

contracts/zero-ex/contracts/src/features/IPuppetPool.sol

+    ///      pool. If one is not available, a new one will be deployed.
+    ///      Only callable from within.
+    /// @return puppet The acquired puppet.
+    function _acquirePuppet() external returns (IPuppet puppet);


Proposing prefixing "internal" registered functions with _.

dorothy-zbornak · 2020-05-05T07:36:15Z

contracts/zero-ex/contracts/src/features/ITransformERC20.sol

+    /// @dev Internal version of `transformERC20()`. Only callable from within.
+    /// @param callDataHash Hash of the ingress calldata.
+    /// @param taker The taker address.
+    /// @param inputToken The token being provided by the sender.
+    ///        If `0xeee...`, ETH is implied and should be provided with the call.`
+    /// @param outputToken The token to be acquired by the sender.
+    ///        `0xeee...` implies ETH.
+    /// @param inputTokenAmount The amount of `inputToken` to take from the sender.
+    /// @param minOutputTokenAmount The minimum amount of `outputToken` the sender
+    ///        must receive for the entire transformation to succeed.
+    /// @return outputTokenAmount The amount of `outputToken` received by the sender.
+    function _transformERC20(
+        bytes32 callDataHash,
+        address payable taker,
+        IERC20TokenV06 inputToken,
+        IERC20TokenV06 outputToken,
+        uint256 inputTokenAmount,
+        uint256 minOutputTokenAmount,
+        Transformation[] calldata transformations
+    )
+        external
+        payable
+        returns (uint256 outputTokenAmount);


I expose this function (internally) so we can wrap it with friendlier market fill functions down the line.

dorothy-zbornak · 2020-05-14T04:02:00Z

contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol

    /// @dev Initializes this feature.
-    /// @param impl The actual address of this feature contract.
    /// @return success Magic bytes if successful.
-    function bootstrap(address impl) external returns (bytes4 success) {


Dunno why I was passing in impl when immutable _implementation was a thing.

dorothy-zbornak · 2020-05-14T04:05:16Z

packages/utils/src/revert_error.ts


 // tslint:disable: max-classes-per-file

-type ArgTypes = string | BigNumber | number | boolean | BigNumber[] | string[] | number[] | boolean[];


Add Numberish array support to revert errors.

dorothy-zbornak · 2020-05-14T04:06:25Z

packages/web3-wrapper/package.json

        "chai-bignumber": "^3.0.0",
        "dirty-chai": "^2.0.1",
-        "ganache-core": "^2.9.0-istanbul.0",
+        "ganache-core": "^2.10.2",


Fixes a weird invalid opcode bug.

dorothy-zbornak · 2020-05-14T04:08:27Z

contracts/zero-ex/contracts/src/migrations/InitialMigration.sol

+    /// @param features Features to bootstrap into the proxy.
    /// @return zeroEx The deployed and configured `ZeroEx` contract.
-    function deploy(address payable owner) public virtual returns (ZeroEx zeroEx) {
+    function deploy(address payable owner, BootstrapFeatures memory features)


So now the migrators won't deploy the feature contracts for you. Turns out some of these are quite large. And they were messing with my geth_call.

…nsform()` and disable hex capitalization linter rule because of prettier conflicts.

contracts/zero-ex/contracts/src/external/Puppet.sol

… `Ownable`.

hysz

Looking good, love the optimizations!

hysz · 2020-05-15T23:43:04Z

contracts/zero-ex/contracts/src/external/IPuppet.sol

+    /// @param callData The call data.
+    /// @param value Ether to attach to the call.
+    /// @return resultData The data returned by the call.
+    function execute(


It could be more explicit to name these call and delegateCall, ex:

IPuppet(address).call(ZRX, transferFromCalldata);

Isn't call a reserved keyword?

Doesn't appear to be, just verified in Remix.

Do we actually want this close resemblance? Semantically they're different and that could lead to confusion.

// Call the `call()` function on `addr`. // `msg.sender` will be `addr`. IPuppet(addr).call(target, callData); // vs // Call a raw payload on `addr`. // `msg.sender` will be `address(this)` addr.call(callData);

And it's even more nuanced with delegatecall:

// Call the `delegatecall()` function on `addr`. // Executes in the context of `addr`. // `msg.sender` will be `address(this)`. IPuppet(addr).delegatecall(target, callData) // vs // Execute the raw `callData` with the code of `addr` // in the context of `this`. // `msg.sender` will be `msg.sender`. addr.delegatecall(callData)

how about execute and delegateExecute?

Let's go with executeCall() and executeDelegateCall() so everyone is left equally unimpressed.

contracts/zero-ex/contracts/src/external/Puppet.sol

hysz · 2020-05-16T00:13:38Z

contracts/zero-ex/contracts/src/features/ITransformERC20.sol

+        // was deployed.
+        // This is used to verify that the transformer was deployed
+        // by a contract we trust.
+        bytes rlpNonce;


I wonder if we should hardcode this for now, since we only have one trusted deployer. Just to simplify the call and reduce the possibility of a client-side error. What do you think?

The nonce isn't a known value. It increments for every contract. If a client screws it up, the tx just reverts.

Yeah I was thinking along the lines of hardcoding the unique nonce into each contract before it's deployed. Then having an automatic check before or after the call, either by querying the transformer or standardizing the return data. Ex:

require( isZeroExTransformer(transformer.getNonce()), "ONLY_ZERO_EX_TRANSFORMERS" ); transformer.transform(...);

OR

returnData = transformer.transform(...); require( isZeroExTransformer(returnData.getNonce()), "ONLY_ZERO_EX_TRANSFORMERS" );

I like the latter. One less call. Just returning the nonce alone should be sufficient to indicate success since the RLP encoding is never empty and always unique.

hysz · 2020-05-16T00:20:32Z

contracts/erc20/contracts/src/v06/IERC20TokenV06.sol

@@ -0,0 +1,95 @@
+/*


I know we've explored this before, but could you remind me why we have to duplicate these files? It would seem cleaner to just bump the version in-place and create a new major version of the package. Additionally, this creates a lot more work for a complete review/audit.

Feels bad bumping up the major version when I've only upgraded a handful of the contracts.

This would also freeze this contract package from the context of its dependents. Lerna will choose to install this package from npm for them. So we could never make changes to the previous version from within the monorepo and have it be reflected in its dependents, because it no longer technically exists in the monorepo.

Thanks, yeah that does sound kind of hectic. Another option could be to keep this in a branch separate from development until the transition is complete, similar to how we used to do for new versions of the Exchange. What do you think, would that be more or less cumbersome?

idk, I think that worked for 3.0 because we weren't immediately productionizing the changes. We'd be folding this stuff into asset-swapper and other packages pretty much right away so we'd be looking for a solution in 2 weeks again.

contracts/zero-ex/contracts/src/features/TokenSpender.sol

hysz · 2020-05-16T01:12:08Z

contracts/zero-ex/contracts/src/features/TransformERC20.sol

+            uint256 takerOutputTokenBalanceAfter =
+                LibERC20Transformer.getTokenBalanceOf(outputToken, taker);
+            if (takerOutputTokenBalanceAfter > takerOutputTokenBalanceBefore) {
+                outputTokenAmount = takerOutputTokenBalanceAfter.safeSub(


It would seem to me that we would always want to compute outputTokenAmount, no?

It'll stay at zero if the balance does not increase. We can't represent decreases with a uint256.

For some more context, suppose minOutputTokenAmount == 0. Then the user doesn't mind how much they receive, but wouldn't we still want to emit and return the value? Additionally, if takerOutputTokenBalanceAfter < takerOutputTokenBalanceBefore then the safeSub will just revert.

Yeah, that makes sense. We probably still want to handle a negative change because SafeMath rich reverts are obnoxiously opaque.

I kinda like having the option of not querying balances twice, e.g. for (un)wrapping WETH it'd be a waste of gas to do so

I do too, but I also think it's pretty useful for this function to return the amount transferred regardless.

contracts/zero-ex/contracts/src/features/TransformERC20.sol

contracts/utils/contracts/src/v06/AuthorizableV06.sol

contracts/zero-ex/contracts/src/features/TokenSpender.sol

moodlezoup · 2020-05-16T00:22:16Z

contracts/zero-ex/contracts/src/features/TokenSpender.sol

+
+
+/// @dev Feature that allows spending token allowances.
+contract TokenSpender is


What's the plan for generalizing this to other standards? Will the next version of this feature add support for 721 etc, or are we going to have a separate feature and allowance target?

Yeah, I'm thinking we'll just update this feature and keep the same allowance target.

contracts/zero-ex/contracts/src/features/TransformERC20.sol

moodlezoup · 2020-05-18T17:28:08Z

contracts/zero-ex/contracts/src/external/Puppet.sol

+
+
+/// @dev A contract that can execute arbitrary calls from its owner.
+contract Puppet is


I kinda prefer FlashWallet? (flash loans so hot right now)

moodlezoup · 2020-05-18T17:36:47Z

contracts/zero-ex/contracts/src/features/TransformERC20.sol

+            uint256 takerOutputTokenBalanceAfter =
+                LibERC20Transformer.getTokenBalanceOf(outputToken, taker);
+            if (takerOutputTokenBalanceAfter > takerOutputTokenBalanceBefore) {
+                outputTokenAmount = takerOutputTokenBalanceAfter.safeSub(


I kinda like having the option of not querying balances twice, e.g. for (un)wrapping WETH it'd be a waste of gas to do so

moodlezoup · 2020-05-18T17:40:15Z

contracts/zero-ex/contracts/src/external/IPuppet.sol

+    /// @param callData The call data.
+    /// @param value Ether to attach to the call.
+    /// @return resultData The data returned by the call.
+    function execute(


how about execute and delegateExecute?

…orm()`.

moodlezoup

awesome stuff, just a few nits 🚀

contracts/zero-ex/contracts/src/errors/LibTransformERC20RichErrors.sol

contracts/zero-ex/contracts/src/features/ITransformERC20.sol

contracts/zero-ex/test/features/token_spender_test.ts

hysz · 2020-05-21T00:20:47Z

contracts/zero-ex/contracts/src/features/TransformERC20.sol

+            outputTokenAmount = takerOutputTokenBalanceAfter.safeSub(
+                takerOutputTokenBalanceBefore
+            );
+        }


I wonder if we should revert if the final balance is less than the initial balance? What's a scenario where the user would want this?

Yeah, I can't really think of a compelling scenario.

We now revert.

hysz

Looks great, so excited about this! I just left one question but other than that looks good to go (~‾▿‾)~

dorothy-zbornak changed the base branch from development to feat/contracts/zero-ex April 9, 2020 17:24

dorothy-zbornak force-pushed the feat/contracts/zero-ex branch 2 times, most recently from af4a61c to 8350c09 Compare April 15, 2020 18:03

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 2 times, most recently from 7a803f9 to 7d5162e Compare April 21, 2020 21:37

dorothy-zbornak force-pushed the feat/contracts/zero-ex branch 3 times, most recently from 001283e to 7df6530 Compare April 22, 2020 02:29

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 6 times, most recently from 68aa72c to 1126a26 Compare April 29, 2020 05:15

dorothy-zbornak changed the base branch from feat/contracts/zero-ex to development April 29, 2020 05:17

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 3 times, most recently from b2da1e9 to 9ed933f Compare April 29, 2020 19:54

dorothy-zbornak changed the base branch from development to fix/contracts-zero-ex/initial-migration-self-destruct April 29, 2020 19:54

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 4 times, most recently from dbb6018 to 4cba718 Compare May 5, 2020 07:22

dorothy-zbornak changed the title ~~ZeroEx: Quote Fill Functions~~ ZeroEx: TransformERC20, TokenSpender, PuppetPool May 5, 2020

dorothy-zbornak changed the base branch from fix/contracts-zero-ex/initial-migration-self-destruct to development May 5, 2020 07:27

dorothy-zbornak commented May 5, 2020

View reviewed changes

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 commented May 5, 2020

View reviewed changes

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from 5189e3d to 6de6076 Compare May 14, 2020 03:59

dorothy-zbornak commented May 14, 2020

View reviewed changes

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch 2 times, most recently from ad91546 to 0afcc07 Compare May 14, 2020 06:33

@0x/contracts-zero-ex: Revamp TransformERC20.

b4c5081

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from 0afcc07 to b4c5081 Compare May 14, 2020 15:43

@0x/contracts-zero-ex: Remove payable from `IERC20Transformer.tra…

3b35af2

…nsform()` and disable hex capitalization linter rule because of prettier conflicts.

dorothy-zbornak commented May 14, 2020

View reviewed changes

contracts/zero-ex/contracts/src/external/Puppet.sol Outdated Show resolved Hide resolved

@0x/contracts-zero-ex: Use immutable owner in Puppet instead of…

0a1a147

… `Ownable`.

dorothy-zbornak mentioned this pull request May 15, 2020

ZeroEx: ERC20 Transformers #2576

Merged

4 tasks

hysz reviewed May 16, 2020

View reviewed changes

moodlezoup reviewed May 18, 2020

View reviewed changes

merklejerk and others added 6 commits May 18, 2020 14:56

@x/utils: Address review feedback.

7f36ced

@0x/contracts-zero-ex: Address review feedback.

2325d5a

@0x/contracts-utils: Address review feedback.

b0147ac

@0x/contracts-zero-ex: Return deployment nonce in transform().

7a6425a

@0x/contracts-zero-ex: Finish returning deployment nonce in `transf…

ef0fffe

…orm()`.

@0x/contracts-zero-ex: Fix doc-gen bug.

c1e34b4

dorothy-zbornak force-pushed the feat/contracts-zero-ex/0x-api-market-fns branch from b3d2f47 to c1e34b4 Compare May 19, 2020 03:51

moodlezoup approved these changes May 19, 2020

View reviewed changes

@0x/contracts-zero-ex: Address review comments.

962dc5e

hysz reviewed May 21, 2020

View reviewed changes

hysz approved these changes May 21, 2020

View reviewed changes

merklejerk added 2 commits May 20, 2020 22:05

@0x/utils: Add NegativeTransformERC20OutputERror

e8ecea3

@0x/contracts-zero-ex: Revert if the taker's output amount decreases.

0c83752

dorothy-zbornak merged commit 2fce332 into development May 21, 2020

dorothy-zbornak deleted the feat/contracts-zero-ex/0x-api-market-fns branch May 21, 2020 02:47


		// tslint:disable: max-classes-per-file

		type ArgTypes = string \| BigNumber \| number \| boolean \| BigNumber[] \| string[] \| number[] \| boolean[];



		/// @dev Feature that allows spending token allowances.
		contract TokenSpender is



		/// @dev A contract that can execute arbitrary calls from its owner.
		contract Puppet is

ZeroEx: TransformERC20, TokenSpender #2545

ZeroEx: TransformERC20, TokenSpender #2545

Uh oh!

Conversation

dorothy-zbornak commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Types of changes

Checklist:

Uh oh!

coveralls commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorothy-zbornak May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorothy-zbornak May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hysz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorothy-zbornak May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorothy-zbornak commented Apr 9, 2020 •

edited

Loading

coveralls commented Apr 29, 2020 •

edited

Loading

dorothy-zbornak May 14, 2020 •

edited

Loading

dorothy-zbornak May 14, 2020 •

edited

Loading

dorothy-zbornak May 18, 2020 •

edited

Loading