allow certain commands to revert (Uniswap#65)

contracts/Router.sol

@@ -37,6 +37,7 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
     uint256 constant SWEEP = 0x09;
 
     uint8 constant FLAG_COMMAND_TYPE_MASK = 0x0f;
+    uint8 constant FLAG_ALLOW_REVERT = 0x80;
     uint8 constant COMMAND_INDICES_OFFSET = 8;
     // the first 32 bytes of a dynamic parameter specify the parameter length
     uint8 constant PARAMS_LENGTH_OFFSET = 32;
@@ -70,7 +71,7 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
         uint8 commandType;
         uint8 flags;
         bytes8 indices;
-        bool success = true;
+        bool success;
 
         bytes memory output;
         uint256 totalBytes;
@@ -83,6 +84,7 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
         // terminates when it has passed the final byte of `commands`
         // each command is 8 bytes, so the end of the loop increments by 8
         for (uint256 byteIndex = PARAMS_LENGTH_OFFSET; byteIndex < totalBytes;) {
+            success = true; // true until false
             assembly {
                 // loads the command at byte number `byteIndex` to process
                 command := mload(add(commands, byteIndex))
@@ -126,9 +128,9 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
                 (uint256 value, bytes memory data) = abi.decode(state.buildInputs(indices), (uint256, bytes));
                 (success, output) = Constants.NFTX_ZAP.call{value: value}(data);
             } else if (commandType == LOOKS_RARE) {
-                callProtocolWithNFTTransfer(inputs, Constants.LOOKS_RARE, byteIndex);
+                (success, output) = callProtocolWithNFTTransfer(inputs, Constants.LOOKS_RARE);
             } else if (commandType == X2Y2) {
-                callProtocolWithNFTTransfer(inputs, Constants.X2Y2, byteIndex);
+                (success, output) = callProtocolWithNFTTransfer(inputs, Constants.X2Y2);
             } else if (commandType == SWEEP) {
                 (address token, address recipient, uint256 minValue) = abi.decode(inputs, (address, address, uint256));
                 Payments.sweepToken(token, recipient, minValue);
@@ -142,7 +144,9 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
                 revert InvalidCommandType({commandIndex: (byteIndex - 32) / 8});
             }
 
-            if (!success) revert ExecutionFailed({commandIndex: (byteIndex - 32) / 8, message: output});
+            if (!success && successRequired(flags)) {
+                revert ExecutionFailed({commandIndex: (byteIndex - 32) / 8, message: output});
+            }
 
             unchecked {
                 byteIndex += 8;
@@ -152,12 +156,18 @@ contract Router is V2SwapRouter, V3SwapRouter, RouterCallbacks {
         return state;
     }
 
-    function callProtocolWithNFTTransfer(bytes memory inputs, address protocol, uint256 byteIndex) internal {
+    function successRequired(uint8 flags) internal pure returns (bool) {
+        return flags & FLAG_ALLOW_REVERT == 0;
+    }
+
+    function callProtocolWithNFTTransfer(bytes memory inputs, address protocol)
+        internal
+        returns (bool success, bytes memory output)
+    {
         (uint256 value, bytes memory data, address recipient, address token, uint256 id) =
             abi.decode(inputs, (uint256, bytes, address, address, uint256));
-        (bool success, bytes memory output) = protocol.call{value: value}(data);
-        if (!success) revert ExecutionFailed({commandIndex: (byteIndex - 32) / 8, message: output});
-        ERC721(token).safeTransferFrom(address(this), recipient, id);
+        (success, output) = protocol.call{value: value}(data);
+        if (success) ERC721(token).safeTransferFrom(address(this), recipient, id);
     }
 
     receive() external payable {

contracts/libraries/CommandBuilder.sol

@@ -6,7 +6,6 @@ library CommandBuilder {
     uint256 constant IDX_VARIABLE_LENGTH = 0x80;
     uint256 constant IDX_VALUE_MASK = 0x7f;
     uint256 constant IDX_END_OF_ARGS = 0xff;
-    uint256 constant IDX_USE_STATE = 0xfe;
 
     function buildInputs(bytes[] memory state, bytes32 indices) internal view returns (bytes memory ret) {
         uint256 count; // Number of bytes in whole ABI encoded message

package.json

@@ -37,7 +37,7 @@
     "@typechain/ethers-v5": "^4.0.0",
     "@types/chai": "^4.2.6",
     "@types/mocha": "^5.2.7",
-    "@uniswap/narwhal-sdk": "1.0.0-beta.5",
+    "@uniswap/narwhal-sdk": "1.0.0-beta.6",
     "@uniswap/router-sdk": "^1.3.0",
     "@uniswap/sdk-core": "^3.0.1",
     "@uniswap/snapshot-gas-cost": "^1.0.0",

test/integration-tests/NFTXRouter.test.ts

@@ -14,6 +14,10 @@ import {
   V3_FACTORY_MAINNET,
   V2_INIT_CODE_HASH_MAINNET,
   V3_INIT_CODE_HASH_MAINNET,
+  NFTX_COVEN_VAULT,
+  NFTX_COVEN_VAULT_ID,
+  NFTX_ERC_1155_VAULT,
+  NFTX_ERC_1155_VAULT_ID,
 } from './shared/constants'
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
 import { expandTo18DecimalsBN } from './shared/helpers'
@@ -22,12 +26,6 @@ const { ethers } = hre
 
 const nftxZapInterface = new ethers.utils.Interface(NFTX_ZAP_ABI)
 
-const COVEN_VAULT = '0xd89b16331f39ab3878daf395052851d3ac8cf3cd'
-const COVEN_VAULT_ID = '333'
-
-const ERC_1155_VAULT = '0x78e09c5ec42d505742a52fd10078a57ea186002a'
-const ERC_1155_VAULT_ID = '61'
-
 describe('NFTX', () => {
   let alice: SignerWithAddress
   let router: Router
@@ -61,10 +59,10 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numCovens = 2
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      COVEN_VAULT_ID,
+      NFTX_COVEN_VAULT_ID,
       numCovens,
       [],
-      [WETH.address, COVEN_VAULT],
+      [WETH.address, NFTX_COVEN_VAULT],
       alice.address,
     ])
 
@@ -82,10 +80,10 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numCovens = 2
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      COVEN_VAULT_ID,
+      NFTX_COVEN_VAULT_ID,
       numCovens,
       [584, 3033],
-      [WETH.address, COVEN_VAULT],
+      [WETH.address, NFTX_COVEN_VAULT],
       alice.address,
     ])
 
@@ -111,10 +109,10 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numTwerkys = 2
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      ERC_1155_VAULT_ID,
+      NFTX_ERC_1155_VAULT_ID,
       numTwerkys,
       [],
-      [WETH.address, ERC_1155_VAULT],
+      [WETH.address, NFTX_ERC_1155_VAULT],
       alice.address,
     ])
 
@@ -132,10 +130,10 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numTwerkys = 1
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      ERC_1155_VAULT_ID,
+      NFTX_ERC_1155_VAULT_ID,
       numTwerkys,
       [44],
-      [WETH.address, ERC_1155_VAULT],
+      [WETH.address, NFTX_ERC_1155_VAULT],
       alice.address,
     ])
 
@@ -154,7 +152,7 @@ describe('NFTX', () => {
     const numCovens = 2
     const saleCost = '476686977628668346'
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      COVEN_VAULT_ID,
+      NFTX_COVEN_VAULT_ID,
       numCovens,
       [584, 3033],
       [WETH.address, '0xd89b16331f39ab3878daf395052851d3ac8cf3cd'],
@@ -176,7 +174,7 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numCovens = 2
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      COVEN_VAULT_ID,
+      NFTX_COVEN_VAULT_ID,
       numCovens,
       [],
       [WETH.address, '0xd89b16331f39ab3878daf395052851d3ac8cf3cd'],
@@ -192,7 +190,7 @@ describe('NFTX', () => {
     const value = expandTo18DecimalsBN(4)
     const numCovens = 2
     const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
-      COVEN_VAULT_ID,
+      NFTX_COVEN_VAULT_ID,
       numCovens,
       [584, 3033],
       [WETH.address, '0xd89b16331f39ab3878daf395052851d3ac8cf3cd'],

test/integration-tests/Router.test.ts

@@ -1,23 +1,40 @@
-import { Router } from '../../typechain'
+import { Router, ERC721 } from '../../typechain'
 import type { Contract } from '@ethersproject/contracts'
+import { BigNumber } from 'ethers'
 import { Pair } from '@uniswap/v2-sdk'
 import { expect } from './shared/expect'
 import { abi as TOKEN_ABI } from '../../artifacts/@openzeppelin/contracts/token/ERC20/IERC20.sol/IERC20.json'
+import { abi as ERC721_ABI } from '../../artifacts/solmate/src/tokens/ERC721.sol/ERC721.json'
+import NFTX_ZAP_ABI from './shared/abis/NFTXZap.json'
 import {
   ALICE_ADDRESS,
+  COVEN_ADDRESS,
   DEADLINE,
+  OPENSEA_CONDUIT_KEY,
   V2_FACTORY_MAINNET,
   V3_FACTORY_MAINNET,
   V2_INIT_CODE_HASH_MAINNET,
   V3_INIT_CODE_HASH_MAINNET,
+  NFTX_COVEN_VAULT,
+  NFTX_COVEN_VAULT_ID,
 } from './shared/constants'
+import { seaportOrders, seaportInterface, getOrderParams } from './shared/protocolHelpers/seaport'
 import { resetFork, WETH, DAI } from './shared/mainnetForkHelpers'
-import { RouterPlanner, TransferCommand, V2ExactInputCommand } from '@uniswap/narwhal-sdk'
+import {
+  RouterCommand,
+  RouterPlanner,
+  SeaportCommand,
+  NFTXCommand,
+  TransferCommand,
+  V2ExactInputCommand,
+} from '@uniswap/narwhal-sdk'
 import { makePair } from './shared/swapRouter02Helpers'
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
 import { expandTo18DecimalsBN } from './shared/helpers'
 import hre from 'hardhat'
+
 const { ethers } = hre
+const nftxZapInterface = new ethers.utils.Interface(NFTX_ZAP_ABI)
 
 describe('Router', () => {
   let alice: SignerWithAddress
@@ -90,5 +107,55 @@ describe('Router', () => {
       commands = commands.concat(invalidCommand)
       await expect(router.execute(DEADLINE, commands, state)).to.be.revertedWith('InvalidCommandType(1)')
     })
+
+    describe('partial fills', async () => {
+      let covenContract: ERC721
+      let nftxValue: BigNumber
+      let numCovens: number
+      let invalidSeaportCommand: RouterCommand
+      let value: BigNumber
+
+      beforeEach(async () => {
+        covenContract = new ethers.Contract(COVEN_ADDRESS, ERC721_ABI, alice) as ERC721
+        // add valid nftx order to planner
+        nftxValue = expandTo18DecimalsBN(4)
+        numCovens = 2
+        const calldata = nftxZapInterface.encodeFunctionData('buyAndRedeem', [
+          NFTX_COVEN_VAULT_ID,
+          numCovens,
+          [],
+          [WETH.address, NFTX_COVEN_VAULT],
+          alice.address,
+        ])
+        planner.add(NFTXCommand(nftxValue, calldata))
+
+        let invalidSeaportOrder = JSON.parse(JSON.stringify(seaportOrders[0]))
+        invalidSeaportOrder.protocol_data.signature = '0xdeadbeef'
+        const { order: seaportOrder, value: seaportValue } = getOrderParams(invalidSeaportOrder)
+        const seaportCalldata = seaportInterface.encodeFunctionData('fulfillOrder', [seaportOrder, OPENSEA_CONDUIT_KEY])
+        invalidSeaportCommand = SeaportCommand(seaportValue, seaportCalldata)
+
+        value = seaportValue.add(nftxValue)
+      })
+
+      it('reverts if no commands are allowed to revert', async () => {
+        planner.add(invalidSeaportCommand)
+
+        const { commands, state } = planner.plan()
+        await expect(router.execute(DEADLINE, commands, state, { value })).to.be.revertedWith(
+          'ExecutionFailed(1, "0x8baa579f")'
+        )
+      })
+
+      it('does not revert if invalid seaport transaction allowed to fail', async () => {
+        planner.add(invalidSeaportCommand.allowRevert())
+        const { commands, state } = planner.plan()
+
+        const covenBalanceBefore = await covenContract.balanceOf(alice.address)
+        await router.execute(DEADLINE, commands, state, { value })
+        const covenBalanceAfter = await covenContract.balanceOf(alice.address)
+        expect(covenBalanceAfter.sub(covenBalanceBefore)).to.eq(numCovens)
+      })
+    })
   })
 })

test/integration-tests/SeaportRouter.test.ts

@@ -5,8 +5,13 @@ import { BigNumber } from 'ethers'
 import { Router } from '../../typechain'
 import { abi as ERC721_ABI } from '../../artifacts/solmate/src/tokens/ERC721.sol/ERC721.json'
 import snapshotGasCost from '@uniswap/snapshot-gas-cost'
+import {
+  seaportOrders,
+  seaportInterface,
+  getAdvancedOrderParams,
+  getOrderParams,
+} from './shared/protocolHelpers/seaport'
 
-import SEAPORT_ABI from './shared/abis/Seaport.json'
 import { resetFork } from './shared/mainnetForkHelpers'
 import {
   ALICE_ADDRESS,
@@ -19,85 +24,8 @@ import {
   V3_INIT_CODE_HASH_MAINNET,
 } from './shared/constants'
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
-import { expandTo18DecimalsBN } from './shared/helpers'
 import hre from 'hardhat'
 const { ethers } = hre
-import fs from 'fs'
-
-const seaportOrders = JSON.parse(
-  fs.readFileSync('test/integration-tests/shared/orders/Seaport.json', { encoding: 'utf8' })
-)
-const seaportInterface = new ethers.utils.Interface(SEAPORT_ABI)
-
-type OfferItem = {
-  itemType: BigNumber // enum
-  token: string // address
-  identifierOrCriteria: BigNumber
-  startAmount: BigNumber
-  endAmount: BigNumber
-}
-
-type ConsiderationItem = OfferItem & {
-  recipient: string
-}
-
-type OrderParameters = {
-  offerer: string // address,
-  offer: OfferItem[]
-  consideration: ConsiderationItem[]
-  orderType: BigNumber // enum
-  startTime: BigNumber
-  endTime: BigNumber
-  zoneHash: string // bytes32
-  salt: BigNumber
-  conduitKey: string // bytes32,
-  totalOriginalConsiderationItems: BigNumber
-}
-
-type APIOrder = {
-  protocol_data: { parameters: OrderParameters & { counter: number }; signature: string }
-}
-
-type Order = {
-  parameters: OrderParameters
-  signature: string
-}
-
-type AdvancedOrder = Order & {
-  numerator: BigNumber // uint120
-  denominator: BigNumber // uint120
-  extraData: string // bytes
-}
-
-function getOrderParams(apiOrder: APIOrder): { order: Order; value: BigNumber } {
-  delete apiOrder.protocol_data.parameters.counter
-  const order = {
-    parameters: apiOrder.protocol_data.parameters,
-    signature: apiOrder.protocol_data.signature,
-  }
-  const value = calculateValue(apiOrder.protocol_data.parameters.consideration)
-  return { order, value }
-}
-
-function getAdvancedOrderParams(apiOrder: APIOrder): { advancedOrder: AdvancedOrder; value: BigNumber } {
-  delete apiOrder.protocol_data.parameters.counter
-  const advancedOrder = {
-    parameters: apiOrder.protocol_data.parameters,
-    numerator: BigNumber.from('1'),
-    denominator: BigNumber.from('1'),
-    signature: apiOrder.protocol_data.signature,
-    extraData: '0x00',
-  }
-  const value = calculateValue(apiOrder.protocol_data.parameters.consideration)
-  return { advancedOrder, value }
-}
-
-function calculateValue(considerations: ConsiderationItem[]): BigNumber {
-  return considerations.reduce(
-    (amt: BigNumber, consideration: ConsiderationItem) => amt.add(consideration.startAmount),
-    expandTo18DecimalsBN(0)
-  )
-}
 
 describe('Seaport', () => {
   let alice: SignerWithAddress

test/integration-tests/__snapshots__/LooksRareRouter.test.ts.snap

@@ -3,6 +3,6 @@
 exports[`LooksRare gas: buy 1 NFT on looks rare 1`] = `
 Object {
   "calldataByteLength": 1540,
-  "gasUsed": 258361,
+  "gasUsed": 258313,
 }
 `;