Feedback from PRB on V2.2 (#883)

* docs: improve wording in NatSpec chore: improve wording comments chore: misc improvements and renames docs: capitalize ID refactor: improve variable names refactor: separate max segment and tranche counts refactor: use for loop instead of while * build: bump OpenZeppelin to v5.0.2 * docs: improve writing in NatSpec perf: optimize "_update" by testing "_isTransferable" last test: apply "inheritdoc" test: define "withdrawAmount" once test: improve function names test: improve writing in comments test: set the license to "UNLICENSED" * refactor: rename "streamingModel" to "sablierModel" docs: improve writing in NatSpec refactor: rename "sablierAddress" to "sablierStringified" * refactor: rename internal create function * chore: singular Check, Effect, Interaction docs: improve writing in custom errors' NatSpec * perf: close #878 docs: improve writing in NatSpec test: apply "seconds" keyword * refactor: reorder checks chore: improve writing in comments test: bring back test in "createWithDurations" test: close #880 test: fuzz cliff time as zero test: improve BTs and associated modifiers * test: remove unused import * test: include map symbol test for lockup tranched docs: _create in NatSpec * revert: bring back GPL V3 license in mocks chore: fix segments reference fix: mistaken "cliffs" array * refactor: update precompiles * test: reuse "createWithTimestamps" modifiers * test: revert max counts to 500 * test: use lockup object * style: correct bad formated natspec * refactor: rename "currentTime" to "blockTimestamp" docs: improve wording in comments perf: use "unchecked" for tranche calculations refactor: rename variables * docs: improve writing in NatSpec * refactor: order functions and fields alphabetically * refactor: rename variable * docs: capitalize IDs chore: update "filter_paths" docs: improve writing in comments refactor: include "streamId" in "SablierV2Lockup_WithdrawToZeroAddress" refactor: alphabetical order * build: bump sphinx * test: remove ERC721Mock and use forge mock * test: update precompiles * chore: say "block timestamp" in comments * refactor: update gas snapshot test: replace "maxUint40" with "maxOfThree" * chore: add trailing slash * build: bump PRBMath --------- Co-authored-by: smol-ninja <[email protected]> Co-authored-by: andreivladbrg <[email protected]>
sablier-labs · Jul 3, 2024 · e049546 · e049546
1 parent cbf8f18
commit e049546
Show file tree

Hide file tree

Showing 146 changed files with 1,674 additions and 1,773 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/.solhint.json b/.solhint.json
@@ -9,7 +9,7 @@
     "func-name-mixedcase": "off",
     "func-visibility": ["error", { "ignoreConstructors": true }],
     "gas-custom-errors": "off",
-    "max-line-length": ["error", 123],
+    "max-line-length": ["error", 124],
     "named-parameters-mapping": "warn",
     "no-empty-blocks": "off",
     "not-rely-on-time": "off",

diff --git a/bun.lockb b/bun.lockb
diff --git a/foundry.toml b/foundry.toml
@@ -2,12 +2,12 @@
   auto_detect_solc = false
   bytecode_hash = "none"
   emv_version = "paris"
-  extra_output = ['storageLayout']
+  extra_output = ["storageLayout"]
   ffi = true
   fs_permissions = [
-    { access = "read", path = "out-optimized" },
     { access = "read", path = "./out" },
-    { access = "read-write", path = "./cache" }
+    { access = "read", path = "./out-optimized" },
+    { access = "read-write", path = "./cache" },
   ]
   gas_reports = [
     "SablierV2LockupDynamic",

diff --git a/package.json b/package.json
@@ -11,11 +11,11 @@
     "url": "https://github.com/sablier-labs/v2-core/issues"
   },
   "dependencies": {
-    "@openzeppelin/contracts": "5.0.0",
-    "@prb/math": "github:PaulRBerg/prb-math#a111d11"
+    "@openzeppelin/contracts": "5.0.2",
+    "@prb/math": "github:PaulRBerg/prb-math#16419e5"
   },
   "devDependencies": {
-    "@sphinx-labs/plugins": "^0.30.6",
+    "@sphinx-labs/plugins": "^0.31.2",
     "forge-std": "github:foundry-rs/forge-std#v1.8.1",
     "prettier": "^2.8.8",
     "solady": "0.0.129",
@@ -67,7 +67,7 @@
     "gas:snapshot": "forge snapshot --mp \"./test/integration/**/*.sol\" --nmt \"test(Fuzz)?_RevertWhen_\\w{1,}?\"",
     "gas:snapshot:optimized": "bun run build:optimized && FOUNDRY_PROFILE=test-optimized forge snapshot --mp \"./test/integration/**/*.sol\" --nmt \"test(Fork)?(Fuzz)?_RevertWhen_\\w{1,}?\"",
     "lint": "bun run lint:sol && bun run prettier:check",
-    "lint:sol": "forge fmt --check && bun solhint \"{script,src,test}/**/*.sol\"",
+    "lint:sol": "forge fmt --check && bun solhint \"{precompiles,script,src,test}/**/*.sol\"",
     "prepack": "bun install && bash ./shell/prepare-artifacts.sh",
     "prettier:check": "prettier --check \"**/*.{json,md,svg,yml}\"",
     "prettier:write": "prettier --write \"**/*.{json,md,svg,yml}\"",

diff --git a/precompiles/Precompiles.sol b/precompiles/Precompiles.sol
diff --git a/remappings.txt b/remappings.txt
@@ -1,6 +1,6 @@
 @openzeppelin/contracts/=node_modules/@openzeppelin/contracts/
 @prb/math/=node_modules/@prb/math/
-@sphinx-labs/contracts/=node_modules/@sphinx-labs/contracts/contracts/foundry
+@sphinx-labs/contracts/=node_modules/@sphinx-labs/contracts/contracts/foundry/
 forge-std/=node_modules/forge-std/
 solady/=node_modules/solady/
 solarray/=node_modules/solarray/
diff --git a/script/Base.s.sol b/script/Base.s.sol
@@ -14,21 +14,23 @@ contract BaseScript is Script, Sphinx {
     /// @dev The Avalanche chain ID.
     uint256 internal constant AVALANCHE_CHAIN_ID = 43_114;
 
-    /// @dev The project name for the Sphinx plugin.
-    string internal constant SPHINX_PROJECT_NAME = "test-test";
-
     /// @dev Included to enable compilation of the script without a $MNEMONIC environment variable.
     string internal constant TEST_MNEMONIC = "test test test test test test test test test test test junk";
 
+    /// @dev The project name for the Sphinx plugin.
+    string internal constant TEST_SPHINX_PROJECT_NAME = "test-test";
+
     /// @dev Needed for the deterministic deployments.
     bytes32 internal constant ZERO_SALT = bytes32(0);
 
     /// @dev The address of the transaction broadcaster.
     address internal broadcaster;
 
-    /// @dev The upper limit on the length of data structures to ensure that transactions stay within the
-    /// block gas limit.
-    uint256 internal maxCount;
+    /// @dev The upper limit on the length of segments to ensure that transactions stay within the block gas limit.
+    uint256 internal maxSegmentCount;
+
+    /// @dev The upper limit on the length of tranches to ensure that transactions stay within the block gas limit.
+    uint256 internal maxTrancheCount;
 
     /// @dev Used to derive the broadcaster's address if $EOA is not defined.
     string internal mnemonic;
@@ -52,13 +54,15 @@ contract BaseScript is Script, Sphinx {
             mnemonic = vm.envOr({ name: "MNEMONIC", defaultValue: TEST_MNEMONIC });
             (broadcaster,) = deriveRememberKey({ mnemonic: mnemonic, index: 0 });
         }
-        sphinxProjectName = vm.envOr({ name: "SPHINX_PROJECT_NAME", defaultValue: SPHINX_PROJECT_NAME });
+        sphinxProjectName = vm.envOr({ name: "SPHINX_PROJECT_NAME", defaultValue: TEST_SPHINX_PROJECT_NAME });
 
-        // Sets `maxCount` to 300 for Avalanche, and 500 for all other chains.
+        // Avalanche has a lower block gas limit than most other chains.
         if (block.chainid == AVALANCHE_CHAIN_ID) {
-            maxCount = 300;
+            maxSegmentCount = 300;
+            maxTrancheCount = 298;
         } else {
-            maxCount = 500;
+            maxSegmentCount = 500;
+            maxTrancheCount = 500;
         }
     }
 
@@ -68,10 +72,11 @@ contract BaseScript is Script, Sphinx {
         vm.stopBroadcast();
     }
 
-    /// @dev Configures the Sphinx plugin to use Sphinx managed deployment for smart contracts.
+    /// @dev Configures the Sphinx plugin to manage the deployment of the contracts.
     /// Refer to https://github.com/sphinx-labs/sphinx/tree/main/docs.
+    ///
     /// CLI example:
-    /// - bun sphinx propose script/DeployCore.s.sol --networks testnets --sig "runSphinx(address)" $ADMIN
+    /// bun sphinx propose script/DeployCore.s.sol --networks testnets --sig "runSphinx(address)" $ADMIN
     function configureSphinx() public override {
         sphinxConfig.mainnets = ["arbitrum", "avalanche", "base", "bnb", "gnosis", "ethereum", "optimism", "polygon"];
         sphinxConfig.orgId = vm.envOr({ name: "SPHINX_ORG_ID", defaultValue: TEST_MNEMONIC });
@@ -88,7 +93,7 @@ contract BaseScript is Script, Sphinx {
     /// - The salt format is "ChainID <chainid>, Version <version>".
     /// - The version is obtained from `package.json` using the `ffi` cheatcode:
     /// https://book.getfoundry.sh/cheatcodes/ffi
-    /// - Requires `jq` CLI tool installed: https://jqlang.github.io/jq/
+    /// - Requires the `jq` CLI installed: https://jqlang.github.io/jq/
     function constructCreate2Salt() public returns (bytes32) {
         string memory chainId = block.chainid.toString();
         string[] memory inputs = new string[](4);

diff --git a/script/DeployCore.s.sol b/script/DeployCore.s.sol
@@ -15,7 +15,7 @@ import { BaseScript } from "./Base.s.sol";
 /// 3. {SablierV2LockupLinear}
 /// 4. {SablierV2LockupTranched}
 contract DeployCore is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(address initialAdmin)
         public
         virtual
@@ -30,7 +30,7 @@ contract DeployCore is BaseScript {
         (lockupDynamic, lockupLinear, lockupTranched, nftDescriptor) = _run(initialAdmin);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(address initialAdmin)
         public
         virtual
@@ -55,8 +55,8 @@ contract DeployCore is BaseScript {
         )
     {
         nftDescriptor = new SablierV2NFTDescriptor();
-        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, nftDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, nftDescriptor, maxSegmentCount);
         lockupLinear = new SablierV2LockupLinear(initialAdmin, nftDescriptor);
-        lockupTranched = new SablierV2LockupTranched(initialAdmin, nftDescriptor, maxCount);
+        lockupTranched = new SablierV2LockupTranched(initialAdmin, nftDescriptor, maxTrancheCount);
     }
 }
diff --git a/script/DeployCore2.s.sol b/script/DeployCore2.s.sol
@@ -14,7 +14,7 @@ import { BaseScript } from "./Base.s.sol";
 /// 2. {SablierV2LockupLinear}
 /// 3. {SablierV2LockupTranched}
 contract DeployCore2 is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor nftDescriptor
@@ -31,7 +31,7 @@ contract DeployCore2 is BaseScript {
         (lockupDynamic, lockupLinear, lockupTranched) = _run(initialAdmin, nftDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor nftDescriptor
@@ -59,8 +59,8 @@ contract DeployCore2 is BaseScript {
             SablierV2LockupTranched lockupTranched
         )
     {
-        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, nftDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, nftDescriptor, maxSegmentCount);
         lockupLinear = new SablierV2LockupLinear(initialAdmin, nftDescriptor);
-        lockupTranched = new SablierV2LockupTranched(initialAdmin, nftDescriptor, maxCount);
+        lockupTranched = new SablierV2LockupTranched(initialAdmin, nftDescriptor, maxTrancheCount);
     }
 }
diff --git a/script/DeployDeterministicCore.s.sol b/script/DeployDeterministicCore.s.sol
@@ -17,7 +17,7 @@ import { BaseScript } from "./Base.s.sol";
 ///
 /// @dev Reverts if any contract has already been deployed.
 contract DeployDeterministicCore is BaseScript {
-    /// @dev Deploy using Forge.
+    /// @dev Deploy via Forge.
     function runBroadcast(address initialAdmin)
         public
         virtual
@@ -32,7 +32,7 @@ contract DeployDeterministicCore is BaseScript {
         (lockupDynamic, lockupLinear, lockupTranched, nftDescriptor) = _run(initialAdmin);
     }
 
-    /// @dev Deploy using Sphinx.
+    /// @dev Deploy via Sphinx.
     function runSphinx(address initialAdmin)
         public
         virtual
@@ -58,8 +58,8 @@ contract DeployDeterministicCore is BaseScript {
     {
         bytes32 salt = constructCreate2Salt();
         nftDescriptor = new SablierV2NFTDescriptor{ salt: salt }();
-        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, nftDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, nftDescriptor, maxSegmentCount);
         lockupLinear = new SablierV2LockupLinear{ salt: salt }(initialAdmin, nftDescriptor);
-        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, nftDescriptor, maxCount);
+        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, nftDescriptor, maxTrancheCount);
     }
 }
diff --git a/script/DeployDeterministicCore2.s.sol b/script/DeployDeterministicCore2.s.sol
@@ -16,7 +16,7 @@ import { BaseScript } from "./Base.s.sol";
 ///
 /// @dev Reverts if any contract has already been deployed.
 contract DeployDeterministicCore2 is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor nftDescriptor
@@ -33,7 +33,7 @@ contract DeployDeterministicCore2 is BaseScript {
         (lockupDynamic, lockupLinear, lockupTranched) = _run(initialAdmin, nftDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor nftDescriptor
@@ -62,8 +62,8 @@ contract DeployDeterministicCore2 is BaseScript {
         )
     {
         bytes32 salt = constructCreate2Salt();
-        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, nftDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, nftDescriptor, maxSegmentCount);
         lockupLinear = new SablierV2LockupLinear{ salt: salt }(initialAdmin, nftDescriptor);
-        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, nftDescriptor, maxCount);
+        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, nftDescriptor, maxTrancheCount);
     }
 }
diff --git a/script/DeployDeterministicLockupDynamic.s.sol b/script/DeployDeterministicLockupDynamic.s.sol
@@ -9,7 +9,7 @@ import { BaseScript } from "./Base.s.sol";
 /// @notice Deploys {SablierV2LockupDynamic} at a deterministic address across chains.
 /// @dev Reverts if the contract has already been deployed.
 contract DeployDeterministicLockupDynamic is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -22,7 +22,7 @@ contract DeployDeterministicLockupDynamic is BaseScript {
         lockupDynamic = _run(initialAdmin, initialNFTDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -43,6 +43,6 @@ contract DeployDeterministicLockupDynamic is BaseScript {
         returns (SablierV2LockupDynamic lockupDynamic)
     {
         bytes32 salt = constructCreate2Salt();
-        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, initialNFTDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic{ salt: salt }(initialAdmin, initialNFTDescriptor, maxSegmentCount);
     }
 }
diff --git a/script/DeployDeterministicLockupLinear.s.sol b/script/DeployDeterministicLockupLinear.s.sol
@@ -9,7 +9,7 @@ import { BaseScript } from "./Base.s.sol";
 /// @dev Deploys {SablierV2LockupLinear} at a deterministic address across chains.
 /// @dev Reverts if the contract has already been deployed.
 contract DeployDeterministicLockupLinear is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -22,7 +22,7 @@ contract DeployDeterministicLockupLinear is BaseScript {
         lockupLinear = _run(initialAdmin, initialNFTDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor

diff --git a/script/DeployDeterministicLockupTranched.s.sol b/script/DeployDeterministicLockupTranched.s.sol
@@ -9,7 +9,7 @@ import { BaseScript } from "./Base.s.sol";
 /// @dev Deploys {SablierV2LockupTranched} at a deterministic address across chains.
 /// @dev Reverts if the contract has already been deployed.
 contract DeployDeterministicLockupTranched is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -22,7 +22,7 @@ contract DeployDeterministicLockupTranched is BaseScript {
         lockupTranched = _run(initialAdmin, initialNFTDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -43,6 +43,6 @@ contract DeployDeterministicLockupTranched is BaseScript {
         returns (SablierV2LockupTranched lockupTranched)
     {
         bytes32 salt = constructCreate2Salt();
-        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, initialNFTDescriptor, maxCount);
+        lockupTranched = new SablierV2LockupTranched{ salt: salt }(initialAdmin, initialNFTDescriptor, maxTrancheCount);
     }
 }
diff --git a/script/DeployDeterministicNFTDescriptor.s.sol b/script/DeployDeterministicNFTDescriptor.s.sol
@@ -8,12 +8,12 @@ import { BaseScript } from "./Base.s.sol";
 /// @dev Deploys {SablierV2NFTDescriptor} at a deterministic address across chains.
 /// @dev Reverts if the contract has already been deployed.
 contract DeployDeterministicNFTDescriptor is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast() public virtual broadcast returns (SablierV2NFTDescriptor nftDescriptor) {
         nftDescriptor = _run();
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx() public virtual sphinx returns (SablierV2NFTDescriptor nftDescriptor) {
         nftDescriptor = _run();
     }

diff --git a/script/DeployLockupDynamic.s.sol b/script/DeployLockupDynamic.s.sol
@@ -7,7 +7,7 @@ import { SablierV2LockupDynamic } from "../src/SablierV2LockupDynamic.sol";
 import { BaseScript } from "./Base.s.sol";
 
 contract DeployLockupDynamic is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -20,7 +20,7 @@ contract DeployLockupDynamic is BaseScript {
         lockupDynamic = _run(initialAdmin, initialNFTDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -40,6 +40,6 @@ contract DeployLockupDynamic is BaseScript {
         internal
         returns (SablierV2LockupDynamic lockupDynamic)
     {
-        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, initialNFTDescriptor, maxCount);
+        lockupDynamic = new SablierV2LockupDynamic(initialAdmin, initialNFTDescriptor, maxSegmentCount);
     }
 }
diff --git a/script/DeployLockupLinear.s.sol b/script/DeployLockupLinear.s.sol
@@ -7,7 +7,7 @@ import { SablierV2LockupLinear } from "../src/SablierV2LockupLinear.sol";
 import { BaseScript } from "./Base.s.sol";
 
 contract DeployLockupLinear is BaseScript {
-    /// @dev Deploy using Forge CLI.
+    /// @dev Deploy via Forge.
     function runBroadcast(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor
@@ -20,7 +20,7 @@ contract DeployLockupLinear is BaseScript {
         lockupLinear = _run(initialAdmin, initialNFTDescriptor);
     }
 
-    /// @dev Deploy using Sphinx CLI.
+    /// @dev Deploy via Sphinx.
     function runSphinx(
         address initialAdmin,
         ISablierV2NFTDescriptor initialNFTDescriptor