initial comments/proposed changes (#1)

* initial comments/proposed changes * remove caching comments * revert bug with else-if * use address _token in _withdraw * ERC6909X questions * add natspec for new Split struct field * updates from in-person discussion * fix: update tests * fix: remove unnecessary comments --------- Co-authored-by: Rooh Afza <[email protected]>
0xSplits · Jan 29, 2024 · 629242c · 629242c
1 parent d881eeb
commit 629242c
Show file tree

Hide file tree

Showing 12 changed files with 450 additions and 657 deletions.
diff --git a/packages/splits-v2/foundry.toml b/packages/splits-v2/foundry.toml
@@ -14,7 +14,7 @@ fs_permissions = [{ access = "read-write", path = "./" }]
 
 [profile.default.fuzz]
 max_test_rejects = 1_000_000
-runs = 50
+runs = 1000
 
 [profile.default.invariant]
 call_override = false

diff --git a/packages/splits-v2/src/SplitsWarehouse.sol b/packages/splits-v2/src/SplitsWarehouse.sol
diff --git a/packages/splits-v2/src/interfaces/ISplitsWarehouse.sol b/packages/splits-v2/src/interfaces/ISplitsWarehouse.sol
@@ -8,7 +8,7 @@ interface ISplitsWarehouse is IERC6909 {
 
     function deposit(address _owner, address _token, uint256 _amount) external payable;
 
-    function batchTransfer(address _token, address[] memory _recipients, uint256[] memory _amounts) external;
+    function batchTransfer(address[] memory _recipients, address _token, uint256[] memory _amounts) external;
 
     function withdraw(address _owner, address _token) external;
 }
diff --git a/packages/splits-v2/src/libraries/Cast.sol b/packages/splits-v2/src/libraries/Cast.sol
@@ -12,8 +12,10 @@ library Cast {
         return uint256(uint160(value));
     }
 
-    function toUint160(uint256 x) internal pure returns (uint160) {
-        if (x >= 1 << 160) revert Overflow();
-        return uint160(x);
+    function toUint160(uint256 x) internal pure returns (uint160 y) {
+        if (x >> 160 != 0) revert Overflow();
+        assembly {
+            y := x
+        }
     }
 }
diff --git a/packages/splits-v2/src/libraries/SplitV2.sol b/packages/splits-v2/src/libraries/SplitV2.sol
@@ -7,7 +7,6 @@ library SplitV2Lib {
     /* -------------------------------------------------------------------------- */
 
     error InvalidSplit_TotalAllocationMismatch();
-    error InvalidSplit_InvalidIncentive();
     error InvalidSplit_LengthMismatch();
 
     /* -------------------------------------------------------------------------- */
@@ -21,12 +20,15 @@ library SplitV2Lib {
      * @param allocations The allocations of the split
      * @param totalAllocation The total allocation of the split
      * @param distributionIncentive The incentive for distribution
+     * @param distributeByPush Whether the split balance should be pushed to recipients
      */
     struct Split {
         address[] recipients;
         uint256[] allocations;
         uint256 totalAllocation;
+        // TODO: should incentive & byPush be saved in storage?
         uint16 distributionIncentive;
+        bool distributeByPush;
     }
 
     /* -------------------------------------------------------------------------- */
@@ -48,19 +50,18 @@ library SplitV2Lib {
     }
 
     function validate(Split calldata _split) internal pure {
-        uint256 numOfRecipients = _split.allocations.length;
-        if (_split.recipients.length != numOfRecipients) {
+        uint256 numOfRecipients = _split.recipients.length;
+        if (_split.allocations.length != numOfRecipients) {
             revert InvalidSplit_LengthMismatch();
         }
-        uint256 totalAllocation;
 
-        for (uint256 i = 0; i < numOfRecipients;) {
+        uint256 totalAllocation;
+        for (uint256 i; i < numOfRecipients;) {
             totalAllocation += _split.allocations[i];
             unchecked {
                 ++i;
             }
         }
-
         if (totalAllocation != _split.totalAllocation) revert InvalidSplit_TotalAllocationMismatch();
     }
 
@@ -76,10 +77,9 @@ library SplitV2Lib {
         amounts = new uint256[](numOfRecipients);
 
         distributorReward = _amount * _split.distributionIncentive / PERCENTAGE_SCALE;
-
         _amount -= distributorReward;
 
-        for (uint256 i = 0; i < numOfRecipients;) {
+        for (uint256 i; i < numOfRecipients;) {
             amounts[i] = _amount * _split.allocations[i] / _split.totalAllocation;
             amountDistributed += amounts[i];
             unchecked {
@@ -100,10 +100,9 @@ library SplitV2Lib {
         amounts = new uint256[](numOfRecipients);
 
         distributorReward = _amount * _split.distributionIncentive / PERCENTAGE_SCALE;
-
         _amount -= distributorReward;
 
-        for (uint256 i = 0; i < numOfRecipients;) {
+        for (uint256 i; i < numOfRecipients;) {
             amounts[i] = _amount * _split.allocations[i] / _split.totalAllocation;
             amountDistributed += amounts[i];
             unchecked {

diff --git a/packages/splits-v2/src/splitters/SplitFactoryV2.sol b/packages/splits-v2/src/splitters/SplitFactoryV2.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity ^0.8.18;
 
+// TODO: do we want to use our clone or the minimal standard?
 import { Clone } from "../libraries/Clone.sol";
 import { SplitV2Lib } from "../libraries/SplitV2.sol";
 import { SplitWalletV2 } from "./SplitWalletV2.sol";
@@ -13,33 +14,11 @@ import { SplitWalletV2 } from "./SplitWalletV2.sol";
 contract SplitFactoryV2 {
     using Clone for address;
 
-    /* -------------------------------------------------------------------------- */
-    /*                                   ERRORS                                   */
-    /* -------------------------------------------------------------------------- */
-
-    error ZeroAddress();
-
     /* -------------------------------------------------------------------------- */
     /*                                   EVENTS                                   */
     /* -------------------------------------------------------------------------- */
 
-    event SplitCreated(address indexed split, CreateSplitParams _split);
-
-    /* -------------------------------------------------------------------------- */
-    /*                                   STRUCT                                   */
-    /* -------------------------------------------------------------------------- */
-
-    /**
-     * @notice CreateSplitParams
-     * @param split Split struct
-     * @param owner Owner of the split
-     * @param creator Creator of the split
-     */
-    struct CreateSplitParams {
-        SplitV2Lib.Split split;
-        address owner;
-        address creator;
-    }
+    event SplitCreated(address indexed split, SplitV2Lib.Split splitParams, address owner, address creator);
 
     /* -------------------------------------------------------------------------- */
     /*                                   STORAGE                                  */
@@ -66,85 +45,112 @@ contract SplitFactoryV2 {
 
     /**
      * @notice Create a new split using create2
-     * @param _createSplitParams CreateSplitParams struct
+     * @param _splitParams Params to create split with
+     * @param _owner Owner of created split
+     * @param _creator Creator of created split
      * @param _salt Salt for create2
      */
     function createSplitDeterministic(
-        CreateSplitParams calldata _createSplitParams,
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
+        address _creator,
         bytes32 _salt
     )
         external
         returns (address split)
     {
-        split = SPLIT_WALLET_IMPLEMENTATION.cloneDeterministic(_getSalt(_createSplitParams, _salt));
+        split = SPLIT_WALLET_IMPLEMENTATION.cloneDeterministic(_getSalt(_splitParams, _owner, _salt));
 
-        SplitWalletV2(split).initialize(_createSplitParams.split, _createSplitParams.owner);
+        SplitWalletV2(split).initialize(_splitParams, _owner);
 
-        emit SplitCreated(split, _createSplitParams);
+        emit SplitCreated(split, _splitParams, _owner, _creator);
     }
 
     /**
      * @notice Create a new split using create
-     * @param _createSplitParams CreateSplitParams struct
+     * @param _splitParams Params to create split with
+     * @param _owner Owner of created split
+     * @param _creator Creator of created split
      */
-    function createSplit(CreateSplitParams calldata _createSplitParams) external returns (address split) {
+    function createSplit(
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
+        address _creator
+    )
+        external
+        returns (address split)
+    {
         split = SPLIT_WALLET_IMPLEMENTATION.clone();
 
-        SplitWalletV2(split).initialize(_createSplitParams.split, _createSplitParams.owner);
+        SplitWalletV2(split).initialize(_splitParams, _owner);
 
-        emit SplitCreated(split, _createSplitParams);
+        emit SplitCreated(split, _splitParams, _owner, _creator);
     }
 
     /**
      * @notice Predict the address of a new split
-     * @param _createSplitParams CreateSplitParams struct
+     * @param _splitParams Params to create split with
+     * @param _owner Owner of created split
      * @param _salt Salt for create2
      */
     function predictDeterministicAddress(
-        CreateSplitParams calldata _createSplitParams,
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
         bytes32 _salt
     )
         external
         view
         returns (address)
     {
-        return _predictDeterministicAddress(_createSplitParams, _salt);
+        return _predictDeterministicAddress(_splitParams, _owner, _salt);
     }
 
     /**
      * @notice Predict the address of a new split and check if it is deployed
-     * @param _createSplitParams CreateSplitParams struct
+     * @param _splitParams Params to create split with
+     * @param _owner Owner of created split
      * @param _salt Salt for create2
      */
     function isDeployed(
-        CreateSplitParams calldata _createSplitParams,
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
         bytes32 _salt
     )
         external
         view
-        returns (address split, bool)
+        returns (address split, bool exists)
     {
-        split = _predictDeterministicAddress(_createSplitParams, _salt);
-        return (split, split.code.length > 0);
+        split = _predictDeterministicAddress(_splitParams, _owner, _salt);
+        exists = split.code.length > 0;
     }
 
     /* -------------------------------------------------------------------------- */
     /*                         PRIVATE/INTERNAL FUNCTIONS                         */
     /* -------------------------------------------------------------------------- */
 
-    function _getSalt(CreateSplitParams calldata _createSplitParams, bytes32 _salt) internal pure returns (bytes32) {
-        return keccak256(bytes.concat(abi.encode(_createSplitParams), _salt));
+    function _getSalt(
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
+        bytes32 _salt
+    )
+        internal
+        pure
+        returns (bytes32)
+    {
+        return keccak256(bytes.concat(abi.encode(_splitParams, _owner), _salt));
     }
 
     function _predictDeterministicAddress(
-        CreateSplitParams calldata _createSplitParams,
+        SplitV2Lib.Split calldata _splitParams,
+        address _owner,
         bytes32 _salt
     )
         internal
         view
         returns (address)
     {
-        return
-            SPLIT_WALLET_IMPLEMENTATION.predictDeterministicAddress(_getSalt(_createSplitParams, _salt), address(this));
+        return SPLIT_WALLET_IMPLEMENTATION.predictDeterministicAddress(
+            _getSalt(_splitParams, _owner, _salt), address(this)
+        );
     }
 }