optimization: [internal-s46] remove the restriction that sync must be…

… called inside lock callback

.forge-snapshots/BinPoolManagerTest#testGasDonate.snap

@@ -1 +1 @@
-118763
+118513

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap

@@ -1 +1 @@
-968494
+968244

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap

@@ -1 +1 @@
-327806
+327556

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap

@@ -1 +1 @@
-337530
+337280

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap

@@ -1 +1 @@
-140081
+139831

.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap

@@ -1 +1 @@
-173046
+172921

.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap

@@ -1 +1 @@
-179074
+178949

.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap

@@ -1 +1 @@
-133077
+132952

.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap

@@ -1 +1 @@
-304488
+304363

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap

@@ -1 +1 @@
-347609
+347359

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap

@@ -1 +1 @@
-163070
+162820

.forge-snapshots/CLPoolManagerTest#donateBothTokens.snap

@@ -1 +1 @@
-163350
+163100

.forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap

@@ -1 +1 @@
-108297
+108172

.forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap

@@ -1 +1 @@
-130864
+130739

.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap

@@ -1 +1 @@
-163225
+163100

.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap

@@ -1 +1 @@
-148644
+148519

.forge-snapshots/VaultBytecodeSize.snap

@@ -1 +1 @@
-7651
+7614

src/Vault.sol

@@ -118,7 +118,7 @@ contract Vault is IVault, VaultToken, Ownable {
         }
     }
 
-    function sync(Currency currency) public override isLocked {
+    function sync(Currency currency) public override {
         if (currency.isNative()) {
             VaultReserve.setVaultReserve(CurrencyLibrary.NATIVE, 0);
         } else {
@@ -156,7 +156,9 @@ contract Vault is IVault, VaultToken, Ownable {
 
     /// @inheritdoc IVault
     function collectFee(Currency currency, uint256 amount, address recipient) external onlyRegisteredApp {
-        if (SettlementGuard.getLocker() != address(0)) revert LockHeld();
+        // prevent transfer between the sync and settle balanceOfs (native settle uses msg.value)
+        (Currency syncedCurrency,) = VaultReserve.getVaultReserve();
+        if (!currency.isNative() && syncedCurrency == currency) revert FeeCurrencySynced();
         reservesOfApp[msg.sender][currency] -= amount;
         currency.transfer(recipient, amount);
     }

src/interfaces/IVault.sol

@@ -27,8 +27,8 @@ interface IVault is IVaultToken {
     /// @notice Thrown when there is no locker
     error NoLocker();
 
-    /// @notice Thrown when lock is held by someone
-    error LockHeld();
+    /// @notice Thrown when collectFee is attempted on a token that is synced.
+    error FeeCurrencySynced();
 
     function isAppRegistered(address app) external returns (bool);
 

test/vault/Vault.t.sol

@@ -467,7 +467,7 @@ contract VaultTest is Test, NoIsolate, GasSnapshot, TokenFixture {
     function test_CollectFee() public noIsolate {
         vault.lock(abi.encodeCall(VaultTest._test_CollectFee, ()));
 
-        // collectFee must be called when vault is unlocked
+        // collectFee can be called no matter vault is locked or not
         vm.prank(address(poolManager1));
         vault.collectFee(currency0, 10 ether, address(poolManager1));
 
@@ -488,6 +488,30 @@ contract VaultTest is Test, NoIsolate, GasSnapshot, TokenFixture {
         assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(poolManager1)), 0 ether);
     }
 
+    function test_CollectFee_WhenVaultLocked() public noIsolate {
+        vault.lock(abi.encodeCall(VaultTest._test_CollectFee_WhenVaultLocked, ()));
+    }
+
+    function _test_CollectFee_WhenVaultLocked() external {
+        currency0.settle(vault, address(this), 10 ether, false);
+        currency1.settle(vault, address(this), 10 ether, false);
+        poolManager1.mockAccounting(poolKey1, -10 ether, -10 ether);
+
+        // before collectFee assert
+        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(vault)), 10 ether);
+        assertEq(vault.reservesOfApp(address(poolKey1.poolManager), currency0), 10 ether);
+        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(poolManager1)), 0 ether);
+
+        // collectFee can be called no matter vault is locked or not
+        vm.prank(address(poolManager1));
+        vault.collectFee(currency0, 10 ether, address(poolManager1));
+
+        // after collectFee assert
+        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(vault)), 0 ether);
+        assertEq(vault.reservesOfApp(address(poolKey1.poolManager), currency0), 0 ether);
+        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(poolManager1)), 10 ether);
+    }
+
     function test_CollectFeeFromRandomUser() public {
         address bob = makeAddr("bob");
         vm.startPrank(bob);
@@ -496,12 +520,13 @@ contract VaultTest is Test, NoIsolate, GasSnapshot, TokenFixture {
         vault.collectFee(currency0, 10 ether, bob);
     }
 
-    function test_CollectFeeWhenVaultIsLocked() public noIsolate {
-        vm.expectRevert(IVault.LockHeld.selector);
-        vault.lock(abi.encodeCall(VaultTest._test_CollectFeeWhenVaultIsLocked, ()));
+    function test_CollectFeeWhenCurrencyIsSynced() public noIsolate {
+        vm.expectRevert(IVault.FeeCurrencySynced.selector);
+        vault.lock(abi.encodeCall(VaultTest._test_CollectFeeWhenCurrencyIsSynced, ()));
     }
 
-    function _test_CollectFeeWhenVaultIsLocked() external {
+    function _test_CollectFeeWhenCurrencyIsSynced() external {
+        vault.sync(currency0);
         vm.prank(address(poolManager1));
         vault.collectFee(currency0, 10 ether, address(poolManager1));
     }

test/vault/VaultSync.t.sol

@@ -124,8 +124,8 @@ contract VaultSyncTest is Test, TokenFixture, GasSnapshot, NoIsolate {
         assertEq(amount, 10 ether);
     }
 
-    function test_sync_NoLock() public {
-        vm.expectRevert(abi.encodeWithSelector(IVault.NoLocker.selector));
+    function test_sync() public {
+        // it's ok to sync without lock
         vault.sync(currency0);
     }