review fixes

contracts/domain/BosonConstants.sol

@@ -125,6 +125,7 @@ string constant INVALID_RANGE_LENGTH = "Range length is too large or zero";
 string constant UNEXPECTED_ERC721_RECEIVED = "Unexpected ERC721 received";
 string constant FEE_AMOUNT_TOO_HIGH = "Fee amount is too high";
 string constant VOUCHER_NOT_RECEIVED = "Voucher not received";
+string constant NEGATIVE_PRICE_NOT_ALLOWED = "Negative price not allowed";
 
 // Revert Reasons: Twin related
 uint256 constant SINGLE_TWIN_RESERVED_GAS = 160000;

contracts/interfaces/IWETH9Like.sol → contracts/interfaces/IWrappedNative.sol

@@ -2,11 +2,11 @@
 pragma solidity 0.8.21;
 
 /**
- * @title IWETH9Like
+ * @title IWrappedNative
  *
  * @notice Provides the minimum interface for native token wrapper
  */
-interface IWETH9Like {
+interface IWrappedNative {
     function withdraw(uint256) external;
 
     function deposit() external payable;

contracts/interfaces/handlers/IBosonSequentialCommitHandler.sol

@@ -15,7 +15,7 @@ import { IERC721Receiver } from "../IERC721Receiver.sol";
  */
 interface IBosonSequentialCommitHandler is IBosonExchangeEvents, IBosonFundsLibEvents, IERC721Receiver {
     /**
-     * @notice Commits to an existing exchange. Price discovery is oflaoaded to external contract.
+     * @notice Commits to an existing exchange. Price discovery is offloaded to external contract.
      *
      * Emits a BuyerCommitted event if successful.
      * Transfers voucher to the buyer address.

contracts/mock/PriceDiscovery.sol

@@ -9,7 +9,7 @@ import "./Foreign721.sol";
 /**
  * @dev Simple price discovery contract used in tests
  *
- * This contract simluates external price discovery mechanism.
+ * This contract simulates external price discovery mechanism.
  * When user commits to an offer, protocol talks to this contract to validate the exchange.
  */
 contract PriceDiscovery {
@@ -24,7 +24,7 @@ contract PriceDiscovery {
 
     /**
      * @dev simple fulfillOrder that does not perform any checks
-     * It just transfers the voucher and exchange token to the buyer
+     * It just transfers the voucher from the seller to the caller (buyer) and exchange token from the caller to the seller
      * If any of the transfers fail, the whole transaction will revert
      */
     function fulfilBuyOrder(Order memory _order) public payable virtual {
@@ -143,7 +143,7 @@ contract PriceDiscoveryModifyVoucherContract is PriceDiscovery {
 /**
  * @dev Simple bad price discovery contract used in tests
  *
- * This contract modifies simply does not transfer the voucher to the caller
+ * This contract simply does not transfer the voucher to the caller
  */
 contract PriceDiscoveryNoTransfer is PriceDiscovery {
     /**

contracts/protocol/bases/PriceDiscoveryBase.sol

@@ -4,7 +4,7 @@ pragma solidity 0.8.21;
 import "../../domain/BosonConstants.sol";
 import { ProtocolLib } from "../libs/ProtocolLib.sol";
 import { IERC20 } from "../../interfaces/IERC20.sol";
-import { IWETH9Like } from "../../interfaces/IWETH9Like.sol";
+import { IWrappedNative } from "../../interfaces/IWrappedNative.sol";
 import { IBosonVoucher } from "../../interfaces/clients/IBosonVoucher.sol";
 import { ProtocolBase } from "./../bases/ProtocolBase.sol";
 import { FundsLib } from "../libs/FundsLib.sol";
@@ -18,10 +18,25 @@ import { Address } from "@openzeppelin/contracts/utils/Address.sol";
 contract PriceDiscoveryBase is ProtocolBase {
     using Address for address;
 
-    IWETH9Like public immutable weth;
+    IWrappedNative public immutable wNative;
+    uint256 private immutable EXCHANGE_ID_2_2_0; // solhint-disable-line
 
-    constructor(address _weth) {
-        weth = IWETH9Like(_weth);
+    /**
+     * @notice
+     * For offers with native exchange token, it is expected the the price discovery contracts will
+     * operate with wrapped native token. Set the address of the wrapped native token in the constructor.
+     *
+     * After v2.2.0, token ids are derived from offerId and exchangeId.
+     * EXCHANGE_ID_2_2_0 is the first exchange id to use for 2.2.0.
+     * Set EXCHANGE_ID_2_2_0 in the constructor.
+     *
+     * @param _wNative - the address of the wrapped native token
+     * @param _firstExchangeId2_2_0 - the first exchange id to use for 2.2.0
+     */
+    //solhint-disable-next-line
+    constructor(address _wNative, uint256 _firstExchangeId2_2_0) {
+        wNative = IWrappedNative(_wNative);
+        EXCHANGE_ID_2_2_0 = _firstExchangeId2_2_0;
     }
 
     /**
@@ -33,20 +48,20 @@ contract PriceDiscoveryBase is ProtocolBase {
      * @param _exchangeToken - the address of the ERC20 token used for the exchange (zero address for native)
      * @param _priceDiscovery - the fully populated BosonTypes.PriceDiscovery struct
      * @param _buyer - the buyer's address (caller can commit on behalf of a buyer)
-     * @param _initialSellerId - the id of the original seller
+     * @param _offer - the pointer to offer struct, corresponding to the exchange
      * @return actualPrice - the actual price of the order
      */
     function fulFilOrder(
         uint256 _exchangeId,
         address _exchangeToken,
         PriceDiscovery calldata _priceDiscovery,
         address _buyer,
-        uint256 _initialSellerId
+        Offer storage _offer
     ) internal returns (uint256 actualPrice) {
         if (_priceDiscovery.side == Side.Ask) {
-            return fulfilAskOrder(_exchangeId, _exchangeToken, _priceDiscovery, _buyer, _initialSellerId);
+            return fulfilAskOrder(_exchangeId, _exchangeToken, _priceDiscovery, _buyer, _offer);
         } else {
-            return fulfilBidOrder(_exchangeId, _exchangeToken, _priceDiscovery, _initialSellerId);
+            return fulfilBidOrder(_exchangeId, _exchangeToken, _priceDiscovery, _offer);
         }
     }
 
@@ -66,15 +81,15 @@ contract PriceDiscoveryBase is ProtocolBase {
      * @param _exchangeToken - the address of the ERC20 token used for the exchange (zero address for native)
      * @param _priceDiscovery - the fully populated BosonTypes.PriceDiscovery struct
      * @param _buyer - the buyer's address (caller can commit on behalf of a buyer)
-     * @param _initialSellerId - the id of the original seller
+     * @param _offer - the pointer to offer struct, corresponding to the exchange
      * @return actualPrice - the actual price of the order
      */
     function fulfilAskOrder(
         uint256 _exchangeId,
         address _exchangeToken,
         PriceDiscovery calldata _priceDiscovery,
         address _buyer,
-        uint256 _initialSellerId
+        Offer storage _offer
     ) internal returns (uint256 actualPrice) {
         // Transfer buyers funds to protocol
         FundsLib.validateIncomingPayment(_exchangeToken, _priceDiscovery.price);
@@ -89,11 +104,11 @@ contract PriceDiscoveryBase is ProtocolBase {
 
         // Store the information about incoming voucher
         ProtocolLib.ProtocolStatus storage ps = protocolStatus();
-        address cloneAddress = protocolLookups().cloneAddress[_initialSellerId];
+        address cloneAddress = getCloneAddress(protocolLookups(), _offer.sellerId, _offer.collectionIndex);
         uint256 tokenId;
         {
-            uint256 offerId = protocolEntities().exchanges[_exchangeId].offerId;
-            tokenId = _exchangeId | (offerId << 128);
+            tokenId = _exchangeId;
+            if (tokenId >= EXCHANGE_ID_2_2_0) tokenId |= (_offer.id << 128);
             ps.incomingVoucherId = tokenId;
             ps.incomingVoucherCloneAddress = cloneAddress;
         }
@@ -115,6 +130,7 @@ contract PriceDiscoveryBase is ProtocolBase {
 
         // Check the escrow amount
         uint256 protocolBalanceAfter = getBalance(_exchangeToken);
+        require(protocolBalanceBefore >= protocolBalanceAfter, NEGATIVE_PRICE_NOT_ALLOWED);
         actualPrice = protocolBalanceBefore - protocolBalanceAfter;
 
         uint256 overchargedAmount = _priceDiscovery.price - actualPrice;
@@ -139,30 +155,33 @@ contract PriceDiscoveryBase is ProtocolBase {
      * @param _exchangeId - the id of the exchange to commit to
      * @param _exchangeToken - the address of the ERC20 token used for the exchange (zero address for native)
      * @param _priceDiscovery - the fully populated BosonTypes.PriceDiscovery struct
-     * @param _initialSellerId - the id of the original seller
+     * @param _offer - the pointer to offer struct, corresponding to the exchange
      * @return actualPrice - the actual price of the order
      */
     function fulfilBidOrder(
         uint256 _exchangeId,
         address _exchangeToken,
         PriceDiscovery calldata _priceDiscovery,
-        uint256 _initialSellerId
+        Offer storage _offer
     ) internal returns (uint256 actualPrice) {
-        IBosonVoucher bosonVoucher = IBosonVoucher(protocolLookups().cloneAddress[_initialSellerId]);
+        IBosonVoucher bosonVoucher = IBosonVoucher(
+            getCloneAddress(protocolLookups(), _offer.sellerId, _offer.collectionIndex)
+        );
 
         // Transfer seller's voucher to protocol
         // Don't need to use safe transfer from, since that protocol can handle the voucher
-        uint256 offerId = protocolEntities().exchanges[_exchangeId].offerId;
-        uint256 tokenId = _exchangeId | (offerId << 128);
+        uint256 tokenId = _exchangeId;
+        if (tokenId >= EXCHANGE_ID_2_2_0) tokenId |= (_offer.id << 128);
         bosonVoucher.transferFrom(msgSender(), address(this), tokenId);
 
-        if (_exchangeToken == address(0)) _exchangeToken = address(weth);
+        if (_exchangeToken == address(0)) _exchangeToken = address(wNative);
 
         // Get protocol balance before the exchange
         uint256 protocolBalanceBefore = getBalance(_exchangeToken);
 
-        // Track native balance just in case if seller send some native currency or price discovery contract does
-        uint256 protocolNativeBalanceBefore = getBalance(address(0));
+        // Track native balance just in case if seller sends some native currency or price discovery contract does
+        // This is the balance that protocol had, before commit to offer was called
+        uint256 protocolNativeBalanceBefore = getBalance(address(0)) - msg.value;
 
         // Approve price discovery contract to transfer voucher. There is no need to reset approval afterwards, since protocol is not the voucher owner anymore
         bosonVoucher.approve(_priceDiscovery.priceDiscoveryContract, tokenId);

contracts/protocol/facets/SequentialCommitHandlerFacet.sol

@@ -19,7 +19,20 @@ import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
 contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDiscoveryBase {
     using Address for address;
 
-    constructor(address _weth) PriceDiscoveryBase(_weth) {}
+    /**
+     * @notice
+     * For offers with native exchange token, it is expected the the price discovery contracts will
+     * operate with wrapped native token. Set the address of the wrapped native token in the constructor.
+     *
+     * After v2.2.0, token ids are derived from offerId and exchangeId.
+     * EXCHANGE_ID_2_2_0 is the first exchange id to use for 2.2.0.
+     * Set EXCHANGE_ID_2_2_0 in the constructor.
+     *
+     * @param _wNative - the address of the wrapped native token
+     * @param _firstExchangeId2_2_0 - the first exchange id to use for 2.2.0
+     */
+    //solhint-disable-next-line
+    constructor(address _wNative, uint256 _firstExchangeId2_2_0) PriceDiscoveryBase(_wNative, _firstExchangeId2_2_0) {}
 
     /**
      * @notice Initializes facet.
@@ -30,7 +43,7 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
     }
 
     /**
-     * @notice Commits to an existing exchange. Price discovery is oflaoaded to external contract.
+     * @notice Commits to an existing exchange. Price discovery is offloaded to external contract.
      *
      * Emits a BuyerCommitted event if successful.
      * Transfers voucher to the buyer address.
@@ -95,8 +108,8 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
         }
 
         // First call price discovery and get actual price
-        // It might be lower tha submitted for buy orders and higher for sell orders
-        uint256 actualPrice = fulFilOrder(_exchangeId, tokenAddress, _priceDiscovery, _buyer, offer.sellerId);
+        // It might be lower than submitted for buy orders and higher for sell orders
+        uint256 actualPrice = fulFilOrder(_exchangeId, tokenAddress, _priceDiscovery, _buyer, offer);
 
         // Calculate the amount to be kept in escrow
         uint256 escrowAmount;
@@ -111,10 +124,9 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
                     : (protocolFees().percentage * actualPrice) / 10000;
 
                 // Calculate royalties
-                (, uint256 royaltyAmount) = IBosonVoucher(protocolLookups().cloneAddress[offer.sellerId]).royaltyInfo(
-                    _exchangeId,
-                    actualPrice
-                );
+                (, uint256 royaltyAmount) = IBosonVoucher(
+                    getCloneAddress(protocolLookups(), offer.sellerId, offer.collectionIndex)
+                ).royaltyInfo(_exchangeId, actualPrice);
 
                 // Verify that fees and royalties are not higher than the price.
                 require((protocolFeeAmount + royaltyAmount) <= actualPrice, FEE_AMOUNT_TOO_HIGH);
@@ -146,17 +158,17 @@ contract SequentialCommitHandlerFacet is IBosonSequentialCommitHandler, PriceDis
                         // If exchange is native currency, seller cannot directly approve protocol to transfer funds
                         // They need to approve wrapper contract, so protocol can pull funds from wrapper
                         // But since protocol otherwise normally operates with native currency, needs to unwrap it (i.e. withdraw)
-                        FundsLib.transferFundsToProtocol(address(weth), seller, escrowAmount);
-                        weth.withdraw(escrowAmount);
+                        FundsLib.transferFundsToProtocol(address(wNative), seller, escrowAmount);
+                        wNative.withdraw(escrowAmount);
                     } else {
                         FundsLib.transferFundsToProtocol(tokenAddress, seller, escrowAmount);
                     }
                 }
             } else {
                 // when bid side, we have full proceeds in escrow. Keep minimal in, return the difference
                 if (tokenAddress == address(0)) {
-                    tokenAddress = address(weth);
-                    if (escrowAmount > 0) weth.withdraw(escrowAmount);
+                    tokenAddress = address(wNative);
+                    if (escrowAmount > 0) wNative.withdraw(escrowAmount);
                 }
 
                 uint256 payout = actualPrice - escrowAmount;

contracts/protocol/libs/FundsLib.sol

@@ -346,7 +346,7 @@ library FundsLib {
     }
 
     /**
-     * @notice Forwared values to increaseAvailableFunds and emits notifies external listeners.
+     * @notice Forwards values to increaseAvailableFunds and emits notifies external listeners.
      *
      * Emits FundsReleased events
      *
@@ -397,6 +397,13 @@ library FundsLib {
         }
     }
 
+    /**
+     * @notice Same as transferFundsToProtocol(address _tokenAddress, address _from, uint256 _amount),
+     * but _from is message sender
+     *
+     * @param _tokenAddress - address of the token to be transferred
+     * @param _amount - amount to be transferred
+     */
     function transferFundsToProtocol(address _tokenAddress, uint256 _amount) internal {
         transferFundsToProtocol(_tokenAddress, EIP712Lib.msgSender(), _amount);
     }
@@ -412,6 +419,7 @@ library FundsLib {
      * - Contract at token address does not support ERC20 function transfer
      * - Available funds is less than amount to be decreased
      *
+     * @param _entityId - id of entity for which funds should be decreased, or 0 for protocol
      * @param _tokenAddress - address of the token to be transferred
      * @param _to - address of the recipient
      * @param _amount - amount to be transferred
@@ -432,6 +440,20 @@ library FundsLib {
         emit FundsWithdrawn(_entityId, _to, _tokenAddress, _amount, EIP712Lib.msgSender());
     }
 
+    /**
+     * @notice Tries to transfer native currency or tokens from the protocol to the recipient.
+     *
+     * Emits ERC20 Transfer event in call stack if ERC20 token is withdrawn and transfer is successful.
+     *
+     * Reverts if:
+     * - Transfer of native currency is not successful (i.e. recipient is a contract which reverted)
+     * - Contract at token address does not support ERC20 function transfer
+     * - Available funds is less than amount to be decreased
+     *
+     * @param _tokenAddress - address of the token to be transferred
+     * @param _to - address of the recipient
+     * @param _amount - amount to be transferred
+     */
     function transferFundsFromProtocol(address _tokenAddress, address payable _to, uint256 _amount) internal {
         // try to transfer the funds
         if (_tokenAddress == address(0)) {

scripts/config/facet-deploy.js

@@ -77,7 +77,10 @@ async function getFacets(config) {
 
   facetArgs["ConfigHandlerFacet"] = { init: ConfigHandlerFacetInitArgs };
   facetArgs["ExchangeHandlerFacet"] = { init: [], constructorArgs: [protocolConfig.EXCHANGE_ID_2_2_0[network]] };
-  facetArgs["SequentialCommitHandlerFacet"] = { init: [], constructorArgs: [protocolConfig.WETH[network]] };
+  facetArgs["SequentialCommitHandlerFacet"] = {
+    init: [],
+    constructorArgs: [protocolConfig.WrappedNative[network], protocolConfig.EXCHANGE_ID_2_2_0[network]],
+  };
 
   // metaTransactionsHandlerFacet initializer arguments.
   const MetaTransactionsHandlerFacetInitArgs = await getMetaTransactionsHandlerFacetInitArgs(

scripts/config/protocol-parameters.js

@@ -78,7 +78,7 @@ module.exports = {
     localhost: 1,
   },
 
-  WETH: {
+  WrappedNative: {
     mainnet: "0x4102621Ac55e068e148Da09151ce92102c952aab", //dummy
     hardhat: "0x4102621Ac55e068e148Da09151ce92102c952aab", //dummy
     localhost: "0x4102621Ac55e068e148Da09151ce92102c952aab", //dummy

test/util/utils.js

@@ -453,7 +453,7 @@ async function setupTestEnvironment(contracts, { bosonTokenAddress, forwarderAdd
   ];
 
   const facetsToDeploy = await getFacetsWithArgs(facetNames, protocolConfig);
-  facetsToDeploy["SequentialCommitHandlerFacet"].constructorArgs = [wethAddress || ZeroAddress];
+  facetsToDeploy["SequentialCommitHandlerFacet"].constructorArgs[0] = wethAddress || ZeroAddress; // update only weth address
 
   // Cut the protocol handler facets into the Diamond
   await deployAndCutFacets(await protocolDiamond.getAddress(), facetsToDeploy, maxPriorityFeePerGas);