misc cleanup

src/Franchiser.sol

@@ -72,6 +72,7 @@ contract Franchiser is IFranchiser, FranchiserImmutableState, Owned {
         // ensures that initialize can only be called once in clones
         if (delegatee_ == address(0)) revert NoDelegatee();
         if (delegatee != address(0)) revert AlreadyInitialized();
+
         owner = msg.sender;
         // only store the delegator if necessary
         if (delegator_ != address(0)) _delegator = delegator_;
@@ -119,28 +120,24 @@ contract Franchiser is IFranchiser, FranchiserImmutableState, Owned {
         onlyDelegatee
         returns (Franchiser franchiser)
     {
-        if (_subDelegatees.length() == maximumSubDelegatees)
-            revert CannotExceedMaximumSubDelegatees(maximumSubDelegatees);
-        if (_subDelegatees.contains(subDelegatee))
-            revert SubDelegateeAlreadyActive(subDelegatee);
-
         franchiser = getFranchiser(subDelegatee);
-        // deploy a new contract if necessary
-        if (!address(franchiser).isContract()) {
-            franchiser = Franchiser(
+        if (!_subDelegatees.contains(subDelegatee)) {
+            if (_subDelegatees.length() == maximumSubDelegatees)
+                revert CannotExceedMaximumSubDelegatees(maximumSubDelegatees);
+            assert(_subDelegatees.add(subDelegatee));
+            if (!address(franchiser).isContract()) {
+                // deploy a new contract if necessary
                 address(franchiserImplementation).cloneDeterministic(
                     getSalt(subDelegatee)
-                )
-            );
-            franchiser.initialize(
-                subDelegatee,
-                maximumSubDelegatees / DECAY_FACTOR
-            );
+                );
+                franchiser.initialize(
+                    subDelegatee,
+                    maximumSubDelegatees / DECAY_FACTOR
+                );
+            }
+            emit SubDelegateeActivated(subDelegatee);
         }
-
-        assert(_subDelegatees.add(subDelegatee));
         ERC20(address(votingToken)).safeTransfer(address(franchiser), amount);
-        emit SubDelegateeActivated(subDelegatee, franchiser);
     }
 
     /// @inheritdoc IFranchiser
@@ -150,6 +147,7 @@ contract Franchiser is IFranchiser, FranchiserImmutableState, Owned {
     ) external returns (Franchiser[] memory franchisers) {
         if (subDelegatees_.length != amounts.length)
             revert ArrayLengthMismatch(subDelegatees_.length, amounts.length);
+
         franchisers = new Franchiser[](subDelegatees_.length);
         unchecked {
             for (uint256 i = 0; i < subDelegatees_.length; i++)
@@ -170,9 +168,15 @@ contract Franchiser is IFranchiser, FranchiserImmutableState, Owned {
         Franchiser franchiser = getFranchiser(subDelegatee);
         if (assumeExistence || _subDelegatees.contains(subDelegatee)) {
             assert(_subDelegatees.remove(subDelegatee));
-            emit SubDelegateeDeactivated(subDelegatee, franchiser);
+            franchiser.recall(address(this));
+            emit SubDelegateeDeactivated(subDelegatee);
         }
-        if (assumeExistence || address(franchiser).isContract())
+        // this condition can only be reached if unSubDelegate is called with a subDelegatee
+        // that has a franchiser contract but isn't currently active - when this is the case,
+        // calling recall is a no-op if the franchiser doesn't have tokens, so it's fine,
+        // but in the very oddd case that the franchiser has received voting tokens out of
+        // band, this will retrieve them silently, which is also fine
+        else if (address(franchiser).isContract())
             franchiser.recall(address(this));
     }
 
@@ -187,15 +191,15 @@ contract Franchiser is IFranchiser, FranchiserImmutableState, Owned {
     /// @inheritdoc IFranchiser
     function recall(address to) external onlyOwner {
         uint256 numberOfSubDelegatees = _subDelegatees.length();
-        unchecked {
-            while (numberOfSubDelegatees != 0)
+        while (numberOfSubDelegatees != 0) {
+            unchecked {
                 _unSubDelegate(
                     // ordering isn't consistent across removals, but this works
                     _subDelegatees.at(--numberOfSubDelegatees),
                     true
                 );
+            }
         }
-
         ERC20(address(votingToken)).safeTransfer(
             to,
             votingToken.balanceOf(address(this))

src/FranchiserFactory.sol

@@ -55,21 +55,17 @@ contract FranchiserFactory is IFranchiserFactory, FranchiserImmutableState {
         returns (Franchiser franchiser)
     {
         franchiser = getFranchiser(msg.sender, delegatee);
-        // deploy a new contract if necessary
         if (!address(franchiser).isContract()) {
-            franchiser = Franchiser(
-                address(franchiserImplementation).cloneDeterministic(
-                    getSalt(msg.sender, delegatee)
-                )
+            // deploy a new contract if necessary
+            address(franchiserImplementation).cloneDeterministic(
+                getSalt(msg.sender, delegatee)
             );
             franchiser.initialize(
                 msg.sender,
                 delegatee,
                 INITIAL_MAXIMUM_SUBDELEGATEES
             );
-            emit NewFranchiser(msg.sender, delegatee, franchiser);
         }
-
         ERC20(address(votingToken)).safeTransferFrom(
             msg.sender,
             address(franchiser),
@@ -84,6 +80,7 @@ contract FranchiserFactory is IFranchiserFactory, FranchiserImmutableState {
     {
         if (delegatees.length != amounts.length)
             revert ArrayLengthMismatch(delegatees.length, amounts.length);
+
         franchisers = new Franchiser[](delegatees.length);
         unchecked {
             for (uint256 i = 0; i < delegatees.length; i++)
@@ -103,6 +100,7 @@ contract FranchiserFactory is IFranchiserFactory, FranchiserImmutableState {
     {
         if (delegatees.length != tos.length)
             revert ArrayLengthMismatch(delegatees.length, tos.length);
+
         unchecked {
             for (uint256 i = 0; i < delegatees.length; i++)
                 recall(delegatees[i], tos[i]);
@@ -154,7 +152,8 @@ contract FranchiserFactory is IFranchiserFactory, FranchiserImmutableState {
     ) external returns (Franchiser[] memory franchisers) {
         if (delegatees.length != amounts.length)
             revert ArrayLengthMismatch(delegatees.length, amounts.length);
-        uint256 amount;
+
+        uint256 amount = 0;
         for (uint256 i = 0; i < delegatees.length; i++) amount += amounts[i];
         permit(amount, deadline, v, r, s);
         franchisers = new Franchiser[](delegatees.length);

src/interfaces/Franchiser/IFranchiserErrors.sol

@@ -20,10 +20,6 @@ interface IFranchiserErrors {
     /// @param maximumSubDelegatees The maximum (and current) number of `subDelegatees`.
     error CannotExceedMaximumSubDelegatees(uint256 maximumSubDelegatees);
 
-    /// @notice Thrown when the `subDelegatee` being added is already active.
-    /// @param subDelegatee The `subDelegatee` being added.
-    error SubDelegateeAlreadyActive(address subDelegatee);
-
     /// @notice Emitted when two array arguments have different cardinalities.
     /// @param length0 The length of the first array argument.
     /// @param length1 The length of the second array argument.

src/interfaces/Franchiser/IFranchiserEvents.sol

@@ -19,17 +19,9 @@ interface IFranchiserEvents {
 
     /// @notice Emitted when a `subDelegatee` is activated.
     /// @param subDelegatee The `subDelegatee`.
-    /// @param franchiser The Franchiser associated with the `subDelegatee`.
-    event SubDelegateeActivated(
-        address indexed subDelegatee,
-        Franchiser franchiser
-    );
+    event SubDelegateeActivated(address indexed subDelegatee);
 
     /// @notice Emitted when a `subDelegatee` is deactivated.
     /// @param subDelegatee The `subDelegatee`.
-    /// @param franchiser The Franchiser associated with the `subDelegatee`.
-    event SubDelegateeDeactivated(
-        address indexed subDelegatee,
-        Franchiser franchiser
-    );
+    event SubDelegateeDeactivated(address indexed subDelegatee);
 }

src/interfaces/FranchiserFactory/IFranchiserFactory.sol

@@ -2,14 +2,12 @@
 pragma solidity ^0.8;
 
 import {IFranchiserFactoryErrors} from "./IFranchiserFactoryErrors.sol";
-import {IFranchiserFactoryEvents} from "./IFranchiserFactoryEvents.sol";
 import {IFranchiserImmutableState} from "../IFranchiserImmutableState.sol";
 import {Franchiser} from "../../Franchiser.sol";
 
 /// @title Interface for the FranchiserFactory contract.
 interface IFranchiserFactory is
     IFranchiserFactoryErrors,
-    IFranchiserFactoryEvents,
     IFranchiserImmutableState
 {
     /// @notice The initial value for the maximum number of `subDelegatee` addresses that a Franchiser

test/Franchiser.t.sol

@@ -139,7 +139,7 @@ contract FranchiserTest is Test, IFranchiserErrors, IFranchiserEvents {
         vm.expectEmit(true, true, false, true, address(expectedFranchiser));
         emit Initialized(address(franchiser), Utils.bob, Utils.carol, 0);
         vm.expectEmit(true, false, false, true, address(franchiser));
-        emit SubDelegateeActivated(Utils.carol, expectedFranchiser);
+        emit SubDelegateeActivated(Utils.carol);
 
         vm.prank(Utils.bob);
         Franchiser returnedFranchiser = franchiser.subDelegate(Utils.carol, 0);
@@ -173,19 +173,12 @@ contract FranchiserTest is Test, IFranchiserErrors, IFranchiserEvents {
         assertEq(daveFranchiser.maximumSubDelegatees(), 0);
     }
 
-    function testSubDelegateRevertsSubDelegateeAlreadyActive() public {
+    function testSubDelegateCanCallTwice() public {
         franchiser.initialize(Utils.alice, Utils.bob, 2);
-        vm.prank(Utils.bob);
+        vm.startPrank(Utils.bob);
         franchiser.subDelegate(Utils.carol, 0);
-
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                SubDelegateeAlreadyActive.selector,
-                Utils.carol
-            )
-        );
-        vm.prank(Utils.bob);
         franchiser.subDelegate(Utils.carol, 0);
+        vm.stopPrank();
     }
 
     function testSubDelegateNonZeroFull() public {
@@ -259,9 +252,21 @@ contract FranchiserTest is Test, IFranchiserErrors, IFranchiserEvents {
         franchiser.initialize(Utils.alice, Utils.bob, 1);
 
         vm.startPrank(Utils.bob);
-        Franchiser returnedFranchiser = franchiser.subDelegate(Utils.carol, 0);
+        franchiser.subDelegate(Utils.carol, 0);
         vm.expectEmit(true, false, false, true, address(franchiser));
-        emit SubDelegateeDeactivated(Utils.carol, returnedFranchiser);
+        emit SubDelegateeDeactivated(Utils.carol);
+        franchiser.unSubDelegate(Utils.carol);
+        vm.stopPrank();
+
+        assertEq(franchiser.subDelegatees(), new address[](0));
+    }
+
+    function testUnSubDelegateCanCallTwice() public {
+        franchiser.initialize(Utils.alice, Utils.bob, 1);
+
+        vm.startPrank(Utils.bob);
+        franchiser.subDelegate(Utils.carol, 0);
+        franchiser.unSubDelegate(Utils.carol);
         franchiser.unSubDelegate(Utils.carol);
         vm.stopPrank();
 

test/FranchiserFactory.t.sol

@@ -3,7 +3,7 @@ pragma solidity 0.8.15;
 
 import {Test, console2} from "forge-std/Test.sol";
 import {IFranchiserFactoryErrors} from "../src/interfaces/FranchiserFactory/IFranchiserFactoryErrors.sol";
-import {IFranchiserFactoryEvents} from "../src/interfaces/FranchiserFactory/IFranchiserFactoryEvents.sol";
+import {IFranchiserEvents} from "../src/interfaces/Franchiser/IFranchiserEvents.sol";
 import {VotingTokenConcrete} from "./VotingTokenConcrete.sol";
 import {IVotingToken} from "../src/interfaces/IVotingToken.sol";
 import {FranchiserFactory} from "../src/FranchiserFactory.sol";
@@ -13,7 +13,7 @@ import {Utils} from "./Utils.sol";
 contract FranchiserFactoryTest is
     Test,
     IFranchiserFactoryErrors,
-    IFranchiserFactoryEvents
+    IFranchiserEvents
 {
     VotingTokenConcrete private votingToken;
     FranchiserFactory private franchiserFactory;
@@ -59,9 +59,8 @@ contract FranchiserFactoryTest is
             Utils.bob
         );
 
-        vm.expectEmit(true, true, false, true, address(franchiserFactory));
-        emit NewFranchiser(Utils.alice, Utils.bob, expectedFranchiser);
-
+        vm.expectEmit(true, true, true, true, address(expectedFranchiser));
+        emit Initialized(address(franchiserFactory), Utils.alice, Utils.bob, 8);
         vm.prank(Utils.alice);
         Franchiser franchiser = franchiserFactory.fund(Utils.bob, 0);
 
@@ -71,6 +70,13 @@ contract FranchiserFactoryTest is
         assertEq(votingToken.delegates(address(franchiser)), Utils.bob);
     }
 
+    function testFundCanCallTwice() public {
+        vm.startPrank(Utils.alice);
+        franchiserFactory.fund(Utils.bob, 0);
+        franchiserFactory.fund(Utils.bob, 0);
+        vm.stopPrank();
+    }
+
     function testFundNonZeroRevertsTRANSFER_FROM_FAILED() public {
         vm.expectRevert(bytes("TRANSFER_FROM_FAILED"));
         franchiserFactory.fund(Utils.bob, 100);