feat: Multiplex + MetaTransaction integration and MetaTransaction Multi-Fee Support [RFQ-795] [LIT-870] #665

patrick-dowell · 2023-02-21T02:09:17Z

Description

There are four big changes included in this PR:

Allow executeMetaTransaction to pay out multiple fees by modifying the data structure MetaTransactionData. This results in the creation of a v2 MetaTransactions feature contract and MetaTransactionV2Data struct.
Inclusion of four new selectors in MetaTransactionsFeatureV2:

_executeMultiplexBatchSellTokenForTokenCall
_executeMultiplexBatchSellTokenForEthCall
_executeMultiplexMultiHopSellTokenForTokenCall
_executeMultiplexMultiHopSellTokenForEthCall

Modifications to Multiplex to account for the possibility that the MultiplexFeature is entered via MetaTransactionFeatureV2. Specifically, we refactor calls to msg.sender into BatchSellParams.payer and MultiHopSellParams.payer
Add a call to multiHopBatchSellOtc to MultiplexFeature and add a conditional for MultiplexSubcall.OTC in Multiplex's _computeHopTarget. To accomplish this, changes were cherrypicked from Feat/multi hop sell otc [LIT-870] #667 at f32e834.

Testing instructions

Run forge test from contracts/zero-ex. There are numerous new local tests and a new forked test that cover the new functionality and contract, located at MetaTransactionV2Test.t.sol, MultiplexMetaTransactionsV2.t.sol, and MultiplexRfqTest.t.sol.

Types of changes

A new feature contract MetaTransactionsFeatureV2 was created, copying logic from MetaTransactionsFeature.
New functionality was added to MetaTransactionsFeatureV2
New functionality was added to MultiplexFeature
forge tests were added for the new feature contract and to test all new functionality.

Checklist:

Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

duncancmt · 2023-02-21T18:02:37Z

contracts/zero-ex/contracts/src/features/interfaces/IMultiplexFeature.sol

+        // The sender of the transaction.
+        address msgSender;


Is this the beneficiary of the metatransaction, or the relayer? We should invent new terminology here so that it's clear.

This naming is used elsewhere, such as openzeppelin's _msgSender(). msg.sender for regular transactions, and the end-user for GSN relayed calls.

So upon as long as this holds everywhere, the naming is ok.

I believe this is needed to disambiguate in the cases of receiving WETH and the user wanting ETH. In which case we need to be the recipient (to unwrap), but things need to be verified for the real user, such as the OTC/RFQ order was for that user.

In a discussion we had earlier today, @duncancmt and @abls and I felt that the best naming would be payer (// the sender of the input token), which kinda mirrors what's going on with recipient - the receiver of the output token.

@dekz I see what you mean. I think this gets a little confusing with OTC/RFQ when the flow of value is bidirectional (equivalently: when the operation being executed is effectively a metatransaction with 2 signers). I concede that this isn't a big deal for the present PR, though.

duncancmt · 2023-02-21T18:03:55Z

contracts/zero-ex/tests/utils/TestUtils.sol

+    address private constant ZERO_ADDRESS = 0x0000000000000000000000000000000000000000;
+


Why not just use address(0)?

contracts/zero-ex/tests/utils/TestUtils.sol

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

duncancmt · 2023-02-21T18:09:43Z

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

+                    // TODO: Remove this field from SignatureValidationError
+                    //       when rich reverts are part of the protocol repo.


This is sadly copy/pasted from the original metatransaction v1. Happy to open a TODO if this is something we actually want to fix.

duncancmt · 2023-02-21T18:16:33Z

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

+        ExternalTransformERC20Args memory args;
+        {
+            bytes memory encodedStructArgs = new bytes(state.mtx.callData.length - 4 + 32);
+            // Copy the args data from the original, after the new struct offset prefix.
+            bytes memory fromCallData = state.mtx.callData;
+            assert(fromCallData.length >= 160);
+            uint256 fromMem;
+            uint256 toMem;
+            assembly {
+                // Prefix the calldata with a struct offset,
+                // which points to just one word over.
+                mstore(add(encodedStructArgs, 32), 32)
+                // Copy everything after the selector.
+                fromMem := add(fromCallData, 36)
+                // Start copying after the struct offset.
+                toMem := add(encodedStructArgs, 64)
+            }
+            LibBytesV06.memCopy(toMem, fromMem, fromCallData.length - 4);
+            // Decode call args for `ITransformERC20Feature.transformERC20()` as a struct.
+            args = abi.decode(encodedStructArgs, (ExternalTransformERC20Args));
+        }


I still think this would be cleaner with the destructive version:

{ bytes memory data = state.mtx.callData; assert(data.length > 4); assembly { let newLen := sub(mload(data), 4) data := add(data, 4) mstore(data, newLen) } state.mtx.callData = data; } args = abi.decode(state.mtx.callData, ExternalTransformERC20Args);

@dekz do you have an opinion?

contracts/zero-ex/tests/MetaTransactionV2Test.t.sol

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

abls · 2023-02-21T19:24:57Z

contracts/zero-ex/tests/MetaTransactionV2Test.t.sol

+    DeployZeroEx.ZeroExDeployed zeroExDeployed;
+    address private constant ZERO_ADDRESS = 0x0000000000000000000000000000000000000000;
+    address private constant USER_ADDRESS = 0x6dc3a54FeAE57B65d185A7B159c5d3FA7fD7FD0F;
+    uint256 private constant USER_KEY = 0x1fc1630343b31e60b7a197a53149ca571ed9d9791e2833337bbd8110c30710ec;


While USER_KEY and address constants were useful when thrown together for that reference script, we should probably have a better system for this in the test utils. A lot of things from that script are not good practice because that script was intended for a single use within an entirely different context.

Regarding improving our utils, a single getSigner function that can return only one signer isn't enough. It is common in our tests that we need multiple signers. Also, maybe we should be able to sign for any of the accounts in BaseTest. Unsure exactly the best solution, but imo all the people writing tests should have a discussion about organization of the shared test utils at some point.

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

dekz · 2023-02-23T01:48:35Z

contracts/zero-ex/contracts/src/features/interfaces/IMultiplexFeature.sol

+        // The sender of the transaction.
+        address msgSender;


This naming is used elsewhere, such as openzeppelin's _msgSender(). msg.sender for regular transactions, and the end-user for GSN relayed calls.

So upon as long as this holds everywhere, the naming is ok.

I believe this is needed to disambiguate in the cases of receiving WETH and the user wanting ETH. In which case we need to be the recipient (to unwrap), but things need to be verified for the real user, such as the OTC/RFQ order was for that user.

…en, multiplex TokenForEth functions to metatransactions, add msgSender field to multiplex params

…sk, and fixing lint issues

duncancmt

As instructed by @patrick-dowell , I re-reviewed this by diffing MetaTransactionsFeatureV2.sol against MetaTransactionsFeature.sol. Seems pretty straightforward. I left a comment about reentrancy; I haven't gone deep on whether this is a real concern, but it's something to think about. Please add a comment addressing this concern before merging.

duncancmt · 2023-02-23T20:10:10Z

contracts/zero-ex/tests/MetaTransactionV2Test.t.sol

+import "@0x/contracts-erc20/src/IEtherToken.sol";
+
+contract MetaTransactionTest is LocalTest {
+    address private constant ZERO_ADDRESS = address(0);


Why is this a named constant?

Just copy/pasted from previous test code that we wrote. I can get rid of it.

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

…or MultiplexSubcall.OTC [#667]

contracts/zero-ex/contracts/src/features/multiplex/MultiplexFeature.sol

contracts/zero-ex/contracts/src/features/multiplex/MultiplexOtc.sol

… minor issues

patrick-dowell · 2023-04-13T08:47:02Z

contracts/zero-ex/tests/MultiplexMetaTransactionsV2.t.sol

+
+        _executeMetaTransaction(
+            abi.encodeWithSelector(
+                zeroExDeployed.zeroEx.multiplexMultiHopSellTokenForToken.selector,


@abls this should be multiplexMultiHopSellTokenForEth.selector

patrick-dowell requested review from elenadimitrova, duncancmt and abls February 21, 2023 02:09

duncancmt reviewed Feb 21, 2023

View reviewed changes

abls requested changes Feb 21, 2023

View reviewed changes

contracts/zero-ex/contracts/src/features/MetaTransactionsFeatureV2.sol Outdated Show resolved Hide resolved

contracts/zero-ex/contracts/src/features/MetaTransactionsFeatureV2.sol Outdated Show resolved Hide resolved

abls reviewed Feb 21, 2023

View reviewed changes

patrick-dowell force-pushed the feat/MetaTransactionV2 branch from 4e317f2 to 16fcf1f Compare February 22, 2023 01:54

elenadimitrova removed their request for review February 22, 2023 04:33

patrick-dowell force-pushed the feat/MetaTransactionV2 branch from 7d43762 to 00399e6 Compare February 22, 2023 05:29

patrick-dowell commented Feb 22, 2023

View reviewed changes

contracts/zero-ex/contracts/src/features/MetaTransactionsFeatureV2.sol Outdated Show resolved Hide resolved

abls mentioned this pull request Feb 22, 2023

Feat/multi hop sell otc [LIT-870] #667

Closed

3 tasks

dekz reviewed Feb 23, 2023

View reviewed changes

patrick-dowell added 10 commits February 23, 2023 02:06

MetaTransactionData changes

f402c96

MetaTransactionV2 creation and forge tests

5cf4cbe

MetaTransactionData changes

2bbf395

MetaTransactionV2 creation and forge tests

865c1f0

add multiplexBatchSellTokenForToken, multiplexMultiHopSellTokenForTok…

cdb94d1

…en, multiplex TokenForEth functions to metatransactions, add msgSender field to multiplex params

Ran prettier to clean up

3b55a88

More linting

15f79fe

Fixing issues with EIP 712 signature, adding test case against MetaMa…

13dd956

…sk, and fixing lint issues

Addressing suggestions from PR reviewers

0f5d832

Complex rebase of test code based on changes in #655

152accf

patrick-dowell force-pushed the feat/MetaTransactionV2 branch from ecefe5b to 152accf Compare February 23, 2023 10:38

duncancmt approved these changes Feb 23, 2023

View reviewed changes

patrick-dowell and others added 6 commits February 23, 2023 13:26

Fixing multiplex test failure

88c9665

add some tests for multiplex metatransactions

4c83711

prettier

b942551

minor test fix

65a2024

cleaning up and adding batchExecuteMetaTransaction tests

217348f

Removing ZERO_ADDRESS

b5c18c2

add multiHopBatchSellOtc to MultiplexFeature, fix _computeHopTarget f…

f32e834

…or MultiplexSubcall.OTC [#667]

patrick-dowell marked this pull request as ready for review February 24, 2023 07:49

patrick-dowell requested a review from dextracker as a code owner February 24, 2023 07:49

fix _computeHopTarget for otc subcalls

a0485ca

duncancmt reviewed Feb 24, 2023

View reviewed changes

contracts/zero-ex/contracts/src/features/multiplex/MultiplexFeature.sol Show resolved Hide resolved

duncancmt reviewed Feb 24, 2023

View reviewed changes

contracts/zero-ex/contracts/src/features/multiplex/MultiplexOtc.sol Outdated Show resolved Hide resolved

Fixing multiHopSellOtcOrder when params.useSelfBalance is true

1728220

abls approved these changes Feb 24, 2023

View reviewed changes

dextracker changed the title ~~feat: Multiplex + MetaTransaction integration and MetaTransaction Multi-Fee Support [RFQ-795]~~ feat: Multiplex + MetaTransaction integration and MetaTransaction Multi-Fee Support [RFQ-795] [LIT-870] Feb 24, 2023

Making executeMetaTransactionV2 nonpayable and addressing a few other…

6e926af

… minor issues

benkream0x mentioned this pull request Mar 31, 2023

MetaTransactionsFeatureV2 and Multiplex Upgrades 0xProject/ZEIPs#96

Open

Forge update

2d40661

patrick-dowell commented Apr 13, 2023

View reviewed changes

Add MetaTransactionsFeatureV2 to exported contracts

8a0c341

duncancmt merged commit ff104e7 into development Apr 19, 2023

duncancmt deleted the feat/MetaTransactionV2 branch April 19, 2023 22:20

abls mentioned this pull request Apr 20, 2023

MetaTransaction V2 Packages #714

Merged

duncancmt mentioned this pull request Apr 24, 2023

Migrate meta tx tests [RFQ-793] #654

Closed

3 tasks

		address private constant ZERO_ADDRESS = 0x0000000000000000000000000000000000000000;

		// TODO: Remove this field from SignatureValidationError
		// when rich reverts are part of the protocol repo.

feat: Multiplex + MetaTransaction integration and MetaTransaction Multi-Fee Support [RFQ-795] [LIT-870] #665

feat: Multiplex + MetaTransaction integration and MetaTransaction Multi-Fee Support [RFQ-795] [LIT-870] #665

Uh oh!

Conversation

patrick-dowell commented Feb 21, 2023 • 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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncancmt 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrick-dowell commented Feb 21, 2023 •

edited

Loading