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

dextracker · 2023-02-22T03:00:13Z

Description

Testing instructions

Types of changes

Checklist:

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

dextracker · 2023-02-22T03:00:59Z

still writing the test but here is the protocol related code

duncancmt

Short and sweet. I like it.

duncancmt · 2023-02-22T19:59:26Z

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

+        }
+
+        uint256 sellAmount = state.outputTokenAmount;
+        // Try filling the Otc order. Swallows reverts.


Why swallow the revert here? Surely we'd want to bubble

ill probably revert to jacob on this one, there was a reason that otc order reverts did not bubble up but i dont remember what it was.

Definitely want to bubble. In batch fill its possible to append more and recover, MultiHop less so

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

duncancmt · 2023-02-22T20:08:11Z

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

+        );
+        // Validate tokens.
+        require(
+            tokens.length >= 2 &&


Perhaps this is a dumb question: why allow tokens to have length > 2?

this tokens array will contain all of the tokens in the swap, i.e. it needs to be more than 2 tokens to perform a multihop sell. we need token A, intermediate token, token B

Your comment made me look into this in more detail. As far as I can tell, that's not enforced anywhere. tokens comes from the subcall data and isn't even signed. As best I can tell, it's totally unconstrained and this require is basically vacuous

Duncan is right about this being basically vacuous, unless there is something I'm missing, you should be checking params.tokens against the makerToken and takerToken of the OtcOrder.

Something like

require( order.takerToken == params.tokens[state.hopIndex] && order.makerToken == params.tokens[state.hopIndex + 1], "MultiplexOtcOrder::_multiHopSellOtcOrder/INVALID_TOKENS" );

and then also remove tokens from the subcall data here

Can we get rid of tokens altogether? Seems like it's not being used now

Edit: reading is hard; I should learn how to do that sometime

patrick-dowell · 2023-02-22T22:49:35Z

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

+                order,
+                signature,
+                sellAmount.safeDowncastToUint128(),
+                msg.sender,


This line is going to conflict with #665

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

dekz · 2023-02-22T22:55:26Z

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

+        }
+
+        uint256 sellAmount = state.outputTokenAmount;
+        // Try filling the Otc order. Swallows reverts.


Definitely want to bubble. In batch fill its possible to append more and recover, MultiHop less so

duncancmt · 2023-02-23T20:39:15Z

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

@@ -488,7 +489,7 @@ contract MultiplexFeature is
    // If `i == 0`, the target is the address which should hold the input
    // tokens prior to executing `calls[0]`. Otherwise, it is the address
    // that should receive `tokens[i]` upon executing `calls[i-1]`.
-    function _computeHopTarget(MultiHopSellParams memory params, uint256 i) private view returns (address target) {
+    function _computeHopTarget(MultiHopSellParams memory params, uint256 i) private  returns (address target) {


Why no longer view?

was emitting an event for my test and had forgot to change it back, should be fixed now

duncancmt · 2023-02-23T20:39:36Z

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

+        );
+        // Validate tokens.
+        require(
+            tokens.length >= 2 &&


duncancmt

LGTM. Please fix the linter complaints

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

…or MultiplexSubcall.OTC [#667]

github-actions · 2023-03-27T08:04:25Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

…ti-Fee Support [RFQ-795] [LIT-870] (#665) * MetaTransactionData changes * MetaTransactionV2 creation and forge tests * MetaTransactionData changes * MetaTransactionV2 creation and forge tests * add multiplexBatchSellTokenForToken, multiplexMultiHopSellTokenForToken, multiplex TokenForEth functions to metatransactions, add msgSender field to multiplex params * Ran prettier to clean up * More linting * Fixing issues with EIP 712 signature, adding test case against MetaMask, and fixing lint issues * Addressing suggestions from PR reviewers * Complex rebase of test code based on changes in #655 * Fixing multiplex test failure * add some tests for multiplex metatransactions * prettier * minor test fix * cleaning up and adding batchExecuteMetaTransaction tests * Removing ZERO_ADDRESS * add multiHopBatchSellOtc to MultiplexFeature, fix _computeHopTarget for MultiplexSubcall.OTC [#667] * fix _computeHopTarget for otc subcalls * Fixing multiHopSellOtcOrder when params.useSelfBalance is true * Making executeMetaTransactionV2 nonpayable and addressing a few other minor issues * Forge update * Add MetaTransactionsFeatureV2 to exported contracts --------- Co-authored-by: abls <[email protected]> Co-authored-by: Duncan Townsend <[email protected]>

github-actions · 2023-05-19T08:03:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dextracker added 2 commits February 21, 2023 21:58

added _multiHopSellOtcOrder and new path for _executeMultiHopSell

4a7a5d1

add barebones test

062e29c

dextracker requested review from duncancmt, abls and patrick-dowell February 22, 2023 03:00

dextracker changed the title ~~Feat/multi hop sell otc~~ Feat/multi hop sell otc [LIT-870] Feb 22, 2023

duncancmt approved these changes Feb 22, 2023

View reviewed changes

duncancmt reviewed Feb 22, 2023

View reviewed changes

patrick-dowell reviewed Feb 22, 2023

View reviewed changes

address nits

50681fc

dekz reviewed Feb 22, 2023

View reviewed changes

dextracker added 4 commits February 23, 2023 15:11

working foundry script to rfqtMultihop, changes to _computeHopTarget()

4aaba03

prettier and lint

29b7095

re-add view function modifier, removed for foundry testing

5da52b0

remove forge-std/Test import

e08ccf5

duncancmt reviewed Feb 23, 2023

View reviewed changes

change MultiHopSellOtc token logic to rely on the OTC order

e0b6a36

dextracker marked this pull request as ready for review February 23, 2023 21:15

remove reference to tokens[] as its not used anymore

bef3761

duncancmt approved these changes Feb 23, 2023

View reviewed changes

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

dextracker added 2 commits February 23, 2023 18:59

reference hop index to determine the taker for an otc multihop

c266217

revert accidental change to uniswapv3 compute

acd6942

patrick-dowell added a commit that referenced this pull request Feb 24, 2023

add multiHopBatchSellOtc to MultiplexFeature, fix _computeHopTarget f…

f32e834

…or MultiplexSubcall.OTC [#667]

patrick-dowell mentioned this pull request Feb 24, 2023

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

Merged

3 tasks

github-actions bot added the stale label Mar 27, 2023

github-actions bot closed this Apr 11, 2023

duncancmt reopened this Apr 11, 2023

github-actions bot removed the stale label Apr 12, 2023

github-actions bot added the stale label May 19, 2023

github-actions bot closed this Jun 2, 2023

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

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

Uh oh!

Conversation

dextracker commented Feb 22, 2023

Description

Testing instructions

Types of changes

Checklist:

Uh oh!

dextracker commented Feb 22, 2023

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!

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!

abls Feb 23, 2023 • 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!

duncancmt Feb 23, 2023 • 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!

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

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!

duncancmt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 27, 2023

Uh oh!

github-actions bot commented May 19, 2023

Uh oh!

Uh oh!

abls Feb 23, 2023 •

edited

Loading

duncancmt Feb 23, 2023 •

edited

Loading

patrick-dowell Feb 22, 2023 •

edited

Loading