Merge pull request #57 from trader-xyz/fix/v4-fees-eth-calc

johnrjj · web-flow · commit b450a341101d · 2022-04-14T00:53:57.000-04:00
Fixes 0x v4 orders that have ETH with fees
diff --git a/.npmignore b/.npmignore
@@ -2,3 +2,6 @@
 .DS_Store
 node_modules
 local-scripts
+nft-swap-sdk-banner.jpg
+example.ts
+example-v4.ts
diff --git a/src/sdk/v4/NftSwapV4.ts b/src/sdk/v4/NftSwapV4.ts
@@ -42,7 +42,6 @@ import type {
   UserFacingERC1155AssetDataSerializedV4,
   UserFacingERC20AssetDataSerializedV4,
   UserFacingERC721AssetDataSerializedV4,
-  VerifyOrderOptionsOverrides,
 } from './types';
 import {
   ERC1155_TRANSFER_FROM_DATA,
@@ -57,9 +56,9 @@ import {
   ORDERBOOK_API_ROOT_URL_PRODUCTION,
   SearchOrdersResponsePayload,
 } from './orderbook';
+import { getWrappedNativeToken } from '../../utils/addresses';
 import { DIRECTION_MAPPING, OrderStatusV4, TradeDirection } from './enums';
 import { CONTRACT_ORDER_VALIDATOR } from './properties';
-import { getWrappedNativeToken } from '../../utils/addresses';
 import { ETH_ADDRESS_AS_ERC20 } from './constants';
 import { ZERO_AMOUNT } from '../../utils/eth';
 
@@ -159,8 +158,6 @@ export interface AdditionalSdkConfig {
   orderbookRootUrl: string;
 }
 
-export const FAKE_ETH_ADDRESS = '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee';
-
 class NftSwapV4 implements INftSwapV4 {
   public provider: BaseProvider;
   public signer: Signer | undefined;
@@ -443,7 +440,7 @@ class NftSwapV4 implements INftSwapV4 {
 
     // Validate that a bid does not use ETH.
     if (direction === TradeDirection.BuyNFT) {
-      if (erc20.tokenAddress.toLowerCase() === FAKE_ETH_ADDRESS) {
+      if (erc20.tokenAddress.toLowerCase() === ETH_ADDRESS_AS_ERC20) {
         throw new Error(
           'NFT Bids cannot use the native token (e.g. ETH). Please use the wrapped native token (e.g. WETH)'
         );
@@ -620,25 +617,36 @@ class NftSwapV4 implements INftSwapV4 {
     );
   };
 
+  isErc20NativeToken = (order: NftOrderV4): boolean => {
+    return order.erc20Token.toLowerCase() === ETH_ADDRESS_AS_ERC20;
+  };
+
   fillSignedOrder = async (
     signedOrder: SignedNftOrderV4,
     fillOrderOverrides?: Partial<FillOrderOverrides>,
     transactionOverrides?: Partial<PayableOverrides>
   ) => {
+    // Only Sell orders can be filled with ETH
+    const canOrderTypeBeFilledWithNativeToken =
+      signedOrder.direction === TradeDirection.SellNFT;
+    // Is ERC20 being traded the native token
+    const isNativeToken = this.isErc20NativeToken(signedOrder);
+    const needsEthAttached =
+      isNativeToken && canOrderTypeBeFilledWithNativeToken;
+    const erc20TotalAmount = this.getErc20TotalIncludingFees(signedOrder);
+
     // do fill
     if ('erc1155Token' in signedOrder) {
       // If maker is selling an NFT, taker wants to 'buy' nft
       if (signedOrder.direction === TradeDirection.SellNFT) {
-        const needsEthAttached =
-          signedOrder.erc20Token.toLowerCase() === ETH_ADDRESS_AS_ERC20;
-
         return this.exchangeProxy.buyERC1155(
           signedOrder,
           signedOrder.signature,
           signedOrder.erc1155TokenAmount,
           '0x',
           {
-            value: needsEthAttached ? signedOrder.erc20TokenAmount : undefined,
+            // If we're filling an order with ETH, be sure to include the value with fees added
+            value: needsEthAttached ? erc20TotalAmount : undefined,
             ...transactionOverrides,
           }
         );
@@ -675,17 +683,14 @@ class NftSwapV4 implements INftSwapV4 {
       }
     } else if ('erc721Token' in signedOrder) {
       // If maker is selling an NFT, taker wants to 'buy' nft
-
       if (signedOrder.direction === TradeDirection.SellNFT) {
-        const needsEthAttached =
-          signedOrder.erc20Token.toLowerCase() === ETH_ADDRESS_AS_ERC20;
-
         return this.exchangeProxy.buyERC721(
           signedOrder,
           signedOrder.signature,
           '0x',
           {
-            value: needsEthAttached ? signedOrder.erc20TokenAmount : undefined,
+            // If we're filling an order with ETH, be sure to include the value with fees added
+            value: needsEthAttached ? erc20TotalAmount : undefined,
             ...transactionOverrides,
           }
         );
@@ -981,19 +986,31 @@ class NftSwapV4 implements INftSwapV4 {
     };
   };
 
+  /**
+   * Convenience function to sum all fees. Total fees denominated in erc20 token amount.
+   * @param order A 0x v4 order (signed or un-signed);
+   * @returns Total summed fees for a 0x v4 order. Amount is represented in Erc20 token units.
+   */
+  getTotalFees = (order: NftOrderV4): BigNumber => {
+    const fees = order.fees;
+    // In 0x v4, fees are additive (not included in the original erc20 amount)
+    let feesTotal = ZERO_AMOUNT;
+    fees.forEach((fee) => {
+      feesTotal = feesTotal.add(BigNumber.from(fee.amount));
+    });
+    return feesTotal;
+  };
+
   /**
    * Calculates total order cost.
    * In 0x v4, fees are additive (i.e. they are not deducted from the order amount, but added on top of)
    * @param order A 0x v4 order;
-   * @returns BigNumber of order total cost in erc20 unit amount
+   * @returns Total cost of an order (base amount + fees). Amount is represented in Erc20 token units. Does not include gas costs.
    */
   getErc20TotalIncludingFees = (order: NftOrderV4): BigNumber => {
     const fees = order.fees;
     // In 0x v4, fees are additive (not included in the original erc20 amount)
-    let feesTotal = ZERO_AMOUNT;
-    fees.forEach((fee) => {
-      feesTotal = ZERO_AMOUNT.add(BigNumber.from(fee.amount));
-    });
+    let feesTotal = this.getTotalFees(order);
     const orderTotalCost = BigNumber.from(order.erc20TokenAmount).add(
       feesTotal
     );
diff --git a/src/sdk/v4/constants.ts b/src/sdk/v4/constants.ts
@@ -57,3 +57,6 @@ export const PROPERTY_ABI = [
 
 export const ETH_ADDRESS_AS_ERC20 =
   '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee';
+
+export const NATIVE_TOKEN_ADDRESS_AS_ERC20 =
+  '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee';
diff --git a/test/v4/fees.test.ts b/test/v4/fees.test.ts
@@ -1,31 +1,27 @@
 import { BigNumber, ethers } from 'ethers';
+import { ETH_ADDRESS_AS_ERC20 } from '../../src/sdk';
 import { NftSwapV4 } from '../../src/sdk/v4/NftSwapV4';
 
 import { SwappableAssetV4 } from '../../src/sdk/v4/types';
 import { SignedERC721OrderStruct } from '../../src/sdk/v4/types';
-import { NULL_ADDRESS } from '../../src/utils/eth';
 
 jest.setTimeout(90 * 1000);
 
 const MAKER_WALLET_ADDRESS = '0xabc23F70Df4F45dD3Df4EC6DA6827CB05853eC9b';
 const MAKER_PRIVATE_KEY =
   'fc5db508b0a52da8fbcac3ab698088715595f8de9cccf2467d51952eec564ec9';
-// NOTE(johnrjj) - NEVER use these private keys for anything of value, testnets only!
 
-const WETH_TOKEN_ADDRESS_TESTNET = '0xc778417e063141139fce010982780140aa0cd5ab';
 const DAI_TOKEN_ADDRESS_TESTNET = '0x31f42841c2db5173425b5223809cf3a38fede360';
 const TEST_NFT_CONTRACT_ADDRESS = '0xf5de760f2e916647fd766b4ad9e85ff943ce3a2b'; // https://ropsten.etherscan.io/token/0xf5de760f2e916647fd766b4ad9e85ff943ce3a2b?a=0xabc23F70Df4F45dD3Df4EC6DA6827CB05853eC9b
 
 const RPC_TESTNET =
   'https://eth-ropsten.alchemyapi.io/v2/is1WqyAFM1nNFFx2aCozhTep7IxHVNGo';
 
 const MAKER_WALLET = new ethers.Wallet(MAKER_PRIVATE_KEY);
-// const TAKER_WALLET = new ethers.Wallet(TAKER_PRIVATE_KEY);
 
 const PROVIDER = new ethers.providers.StaticJsonRpcProvider(RPC_TESTNET);
 
 const MAKER_SIGNER = MAKER_WALLET.connect(PROVIDER);
-// const TAKER_PROVIDER = TAKER_WALLET.connect(PROVIDER);
 
 const ROPSTEN_CHAIN_ID = 3;
 
@@ -34,26 +30,31 @@ const nftSwapperMaker = new NftSwapV4(
   MAKER_SIGNER,
   ROPSTEN_CHAIN_ID
 );
-// const nftSwapperTaker = new NftSwap(TAKER_PROVIDER as any, 4);
 
-const MAKER_ASSET: SwappableAssetV4 = {
+const DAI_ASSET: SwappableAssetV4 = {
   type: 'ERC20',
   tokenAddress: DAI_TOKEN_ADDRESS_TESTNET,
   amount: '420000000000000', // 1 USDC
 };
 
-const TAKER_ASSET: SwappableAssetV4 = {
+const ETH_ASSET: SwappableAssetV4 = {
+  type: 'ERC20',
+  tokenAddress: ETH_ADDRESS_AS_ERC20,
+  amount: '420000000000000', // 1 USDC
+};
+
+const NFT_ASSET: SwappableAssetV4 = {
   type: 'ERC721',
   tokenAddress: TEST_NFT_CONTRACT_ADDRESS,
   tokenId: '11045',
 };
 
 describe('NFTSwapV4', () => {
-  it('fees', async () => {
+  it('erc20 fee', async () => {
     // NOTE(johnrjj) - Assumes USDC and DAI are already approved w/ the ExchangeProxy
     const v4Erc721Order = nftSwapperMaker.buildOrder(
-      TAKER_ASSET,
-      MAKER_ASSET,
+      NFT_ASSET,
+      DAI_ASSET,
       MAKER_WALLET_ADDRESS,
       {
         fees: [
@@ -63,30 +64,47 @@ describe('NFTSwapV4', () => {
           },
         ],
       }
-      // {
-      //   // Fix dates and salt so we have reproducible tests
-      //   expiration: new Date(3000, 10, 1),
-      // }
     );
 
-    // console.log('v4Erc721Order.nonce', v4Erc721Order.nonce.toString());
+    const signedOrder = (await nftSwapperMaker.signOrder(
+      v4Erc721Order
+    )) as SignedERC721OrderStruct;
 
-    // expect(v4Erc721Order.nonce.toString()./includes('-')).toBeFalsy();
+    expect(signedOrder.fees[0].recipient).toEqual(
+      '0xaaa1388cD71e88Ae3D8432f16bed3c603a58aD34'.toLowerCase()
+    );
 
-    // const makerapprovalTx = await nftSwapperMaker.approveTokenOrNftByAsset(
-    //   MAKER_ASSET,
-    //   MAKER_WALLET_ADDRESS,
-    // )
-    // const makerApprovalTxHash = await (await makerapprovalTx.wait()).transactionHash
-    //   console.log('maker approval tx hash', makerApprovalTxHash)
+    // Ensure getErc20TotalIncludingFees helper function works properly w/ fees.
+    const total = nftSwapperMaker
+      .getErc20TotalIncludingFees(signedOrder)
+      .toString();
+    const handCountedTotal = BigNumber.from(signedOrder.erc20TokenAmount).add(
+      BigNumber.from(signedOrder.fees[0].amount)
+    );
+    expect(total).toBe(handCountedTotal.toString());
 
-    // const takerApprovalTx = await nftSwapperMaker.approveTokenOrNftByAsset(
-    //   TAKER_ASSET,
-    //   MAKER_WALLET_ADDRESS,
-    // )
+    // // Uncomment to actually fill order
+    // const tx = await nftSwapperMaker.fillSignedOrder(signedOrder);
+    // const txReceipt = await tx.wait();
+    // expect(txReceipt.transactionHash).toBeTruthy();
+    // console.log(`Swapped tx with fees on Ropsten (txHAsh: ${txReceipt.transactionHash})`);
+  });
 
-    // const takerApprovalTxHash = await (await takerApprovalTx.wait()).transactionHash
-    // console.log('taker approval tx hash', takerApprovalTxHash)
+  it('eth w/ fee', async () => {
+    // NOTE(johnrjj) - Assumes USDC and DAI are already approved w/ the ExchangeProxy
+    const v4Erc721Order = nftSwapperMaker.buildOrder(
+      NFT_ASSET,
+      ETH_ASSET,
+      MAKER_WALLET_ADDRESS,
+      {
+        fees: [
+          {
+            amount: '6900000000000',
+            recipient: '0xaaa1388cD71e88Ae3D8432f16bed3c603a58aD34',
+          },
+        ],
+      }
+    );
 
     const signedOrder = (await nftSwapperMaker.signOrder(
       v4Erc721Order
@@ -105,41 +123,66 @@ describe('NFTSwapV4', () => {
     );
     expect(total).toBe(handCountedTotal.toString());
 
-    // console.log('erc721 signatuee', signedOrder.signature);
-    // expect(signedOrder.signature.signatureType.toString()).toEqual('2');
-
-    // const fillTx = await nftSwapperMaker.fillSignedCollectionOrder(
-    //   signedOrder,
-    //   '11045',
-    //   {
-    //     fillOrderWithNativeTokenInsteadOfWrappedToken: false,
-    //     tokenIdToSellForCollectionOrder: '11045',
-    //   },
-    //   { gasLimit: '500000' }
+    // Uncomment to actually fill order
+    // const tx = await nftSwapperMaker.fillSignedOrder(signedOrder);
+    // const txReceipt = await tx.wait();
+    // expect(txReceipt.transactionHash).toBeTruthy();
+    // console.log(
+    //   `Swapped tx eth with fees on Ropsten (txHAsh: ${txReceipt.transactionHash})`
     // );
-    // const txReceipt = await fillTx.wait();
-    // console.log('erc721 fill tx', txReceipt.transactionHash);
+  });
 
-    // expect(txReceipt.transactionHash).toBeTruthy();
+  it('eth w/ multiple fees', async () => {
+    // NOTE(johnrjj) - Assumes USDC and DAI are already approved w/ the ExchangeProxy
+    const v4Erc721Order = nftSwapperMaker.buildOrder(
+      NFT_ASSET,
+      ETH_ASSET,
+      MAKER_WALLET_ADDRESS,
+      {
+        fees: [
+          {
+            amount: '6900000000000',
+            recipient: '0xaaa1388cD71e88Ae3D8432f16bed3c603a58aD34',
+          },
+          {
+            amount: '7000000000000',
+            recipient: '0xbbb5A0ceB2344B6566a8e945872D2Ba8Fb04E58E',
+          },
+        ],
+      }
+    );
 
-    // const normalizedOrder = normalizeOrder(order);
-    // const signedOrder = await nftSwapperMaker.signOrder(
-    //   normalizedOrder,
-    // );
+    const signedOrder = (await nftSwapperMaker.signOrder(
+      v4Erc721Order
+    )) as SignedERC721OrderStruct;
 
-    // const normalizedSignedOrder = normalizeOrder(signedOrder);
+    expect(signedOrder.fees[0].recipient).toEqual(
+      '0xaaa1388cD71e88Ae3D8432f16bed3c603a58aD34'.toLowerCase()
+    );
+    expect(signedOrder.fees[1].recipient).toEqual(
+      '0xbbb5A0ceB2344B6566a8e945872D2Ba8Fb04E58E'.toLowerCase()
+    );
 
-    // expect(normalizedSignedOrder.makerAddress.toLowerCase()).toBe(
-    //   MAKER_WALLET_ADDRESS.toLowerCase()
-    // );
+    // Ensure getErc20TotalIncludingFees helper function works properly w/ fees.
+    const computedTotal = nftSwapperMaker
+      .getErc20TotalIncludingFees(signedOrder)
+      .toString();
 
-    // Uncomment to actually fill order
-    // const tx = await nftSwapperMaker.fillSignedOrder(signedOrder); //, undefined, { gasLimit: '500000'})
+    expect(computedTotal).toBe('433900000000000');
+
+    const handCountedTotal = BigNumber.from(signedOrder.erc20TokenAmount)
+      .add(BigNumber.from(signedOrder.fees[0].amount))
+      .add(BigNumber.from(signedOrder.fees[1].amount))
+      .toString();
 
+    expect(handCountedTotal).toBe('433900000000000');
+
+    // Uncomment to actually fill order
+    // const tx = await nftSwapperMaker.fillSignedOrder(signedOrder);
     // const txReceipt = await tx.wait();
     // expect(txReceipt.transactionHash).toBeTruthy();
-    // console.log(`Swapped on Rinkeby (txHAsh: ${txReceipt.transactionHash})`);
+    // console.log(
+    //   `Swapped tx eth with multiple fees on Ropsten (txHAsh: ${txReceipt.transactionHash})`
+    // );
   });
 });
-
-// https://polygon-mumbai.g.alchemy.com/v2/VMBpFqjMYv2w-MWnc9df92w3R2TpMvSG
