Add transfer simulator and reorder Exchange transfers #1868
Conversation
contracts/exchange/test/core.ts
Outdated
| @@ -418,6 +382,167 @@ describe('Exchange core', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe.only('Fill transfer ordering', () => { | |||
There was a problem hiding this comment.
| describe.only('Fill transfer ordering', () => { | |
| describe('Fill transfer ordering', () => { |
abandeali1
force-pushed
the
feat/3.0/transferSimulator
branch
from
7191d76 to
8b2f64a
Compare
abandeali1
force-pushed
the
feat/3.0/transferSimulator
branch
from
8b2f64a to
e959dd1
Compare
| } else if (keccak256(returnData) == _TRANSFERS_SUCCESSFUL_RESULT_HASH) { | ||
| // All transfers were successful | ||
| return OrderTransferResults.TransfersSuccessful; | ||
| } |
There was a problem hiding this comment.
Nit - it could be good to add a catch-all else here to show that we've considered all cases + for future-proofing.
| { | ||
| uint256 length = assetData.length; | ||
| for (uint256 i = 0; i != length; i++) { | ||
| _dispatchTransferFrom( |
There was a problem hiding this comment.
Somewhere it may be helpful to note that passing in the index as the order hash is not simply filler, but necessary for getSimulatedOrderTransferResults to retrieve the index of the failed transfer.
| @@ -336,39 +335,34 @@ contract MixinMatchOrders is | |||
| ) { | |||
| // Fee recipients and taker fee assets are identical, so we can | |||
| // transfer them in one go. | |||
| if (takerAddress != leftFeeRecipientAddress) { | |||
| /// As they would occur through the Exchange contract. Note that this function | ||
| /// will always revert, even if all transfers are successful. However, it may | ||
| /// be used with eth_call or with a try/catch pattern in order to simulate | ||
| /// the results of the transfers. |
There was a problem hiding this comment.
This is an interesting design pattern. Why is this better than reverting only when there is an error?
There was a problem hiding this comment.
I think we would need a way to detect we're in an eth_call context otherwise.
There was a problem hiding this comment.
Because it's a public function with access to assets, so we want to ensure that it always reverts state when called :) The purpose of this is to allow validation of arbitrary transfers within a single RPC call (people have been experiencing rate limiting issues + this will speed up validation a good amount).
Description
MixinTransferSimulator, which performs a series of transfers and then always reverts. Example usage can be found inOrderTransferSimulationUtils.OrderTransferSimulationUtils, which allows batch order transfer simulations.fillOrderandmatchOrderssuch that the maker can spend the takerAsset and such that both the maker/taker can spend received assets on fees.-Removes some redundant checks done before transfers.
dispatchTransferFromwill already not perform a transfer iffrom == to.Testing instructions
Types of changes
Checklist:
[WIP]if necessary.