Merge pull request #116 from decentdao/create-roles-race

Fix create roles race condition

.prettierrc

@@ -7,6 +7,7 @@
   "semi": true,
   "arrowParens": "avoid",
   "singleAttributePerLine": true,
+  "trailingComma": "all",
   "overrides": [
     {
       "files": "*.sol",

contracts/DecentHats_0_1_0.sol

@@ -49,7 +49,7 @@ contract DecentHats_0_1_0 {
             0x5d0e6ce4fd951366cc55da93f6e79d8b81483109d79676a04bcc2bed6a4b5072;
     }
 
-    function updateKeyValuePairs(
+    function declareSafeHatTree(
         address _keyValuePairs,
         uint256 topHatId
     ) internal {
@@ -195,6 +195,54 @@ contract DecentHats_0_1_0 {
         }
     }
 
+    /**
+     * Creates a new role hat and any streams on it.
+     *
+     * This contract should be enabled a module on the Safe for which the role is to be created, and disable after.
+     * In order for the module to be able to create hats on behalf of the Safe, the Safe must first
+     * transfer its top hat to this contract. This function transfers the top hat back to the Safe after
+     * creating the role hat.
+     *
+     * The function simply calls `createHatAndAccountAndMintAndStreams` and then transfers the top hat back to the Safe.
+     *
+     * @dev Role hat creation, minting, smart account creation and stream creation are handled here in order
+     * to avoid a race condition where not more than one active proposal to create a new role can exist at a time.
+     * See: https://github.com/decentdao/decent-interface/issues/2402
+     */
+    function createRoleHat(
+        IHats hatsProtocol,
+        uint256 adminHatId,
+        Hat calldata hat,
+        uint256 topHatId,
+        address topHatAccount,
+        IERC6551Registry registry,
+        address hatsAccountImplementation,
+        bytes32 salt
+    ) public returns (uint256 hatId, address accountAddress) {
+        (hatId, accountAddress) = createHatAndAccountAndMintAndStreams(
+            hatsProtocol,
+            adminHatId,
+            hat,
+            topHatAccount,
+            registry,
+            hatsAccountImplementation,
+            salt
+        );
+
+        hatsProtocol.transferHat(topHatId, address(this), msg.sender);
+    }
+
+    /**
+     * For a safe without any roles previously created on it, this function should be called. It sets up the
+     * top hat and admin hat, as well as any other hats and their streams that are provided.
+     *
+     * This contract should be enabled a module on the Safe for which the role(s) are to be created, and disabled after.
+     *
+     * @dev In order for a Safe to seamlessly create roles even if it has never previously created a role and thus has
+     * no hat tree, we defer the creation of the hat tree and its setup to this contract. This way, in a single tx block,
+     * the resulting topHatId of the newly created hat can be used to create an admin hat and any other hats needed.
+     * We also make use of `KeyValuePairs` to associate the topHatId with the Safe.
+     */
     function createAndDeclareTree(CreateTreeParams calldata params) public {
         bytes32 salt = getSalt();
 
@@ -207,7 +255,7 @@ contract DecentHats_0_1_0 {
             salt
         );
 
-        updateKeyValuePairs(params.keyValuePairs, topHatId);
+        declareSafeHatTree(params.keyValuePairs, topHatId);
 
         (uint256 adminHatId, ) = createHatAndAccountAndMintAndStreams(
             params.hatsProtocol,

contracts/interfaces/hats/IHats.sol

@@ -39,4 +39,9 @@ interface IHats {
     ) external returns (bool success);
 
     function transferHat(uint256 _hatId, address _from, address _to) external;
+
+    function isWearerOfHat(
+        address _user,
+        uint256 _hatId
+    ) external view returns (bool isWearer);
 }

contracts/mocks/MockHats.sol

@@ -5,14 +5,16 @@ import {IHats} from "../interfaces/hats/IHats.sol";
 
 contract MockHats is IHats {
     uint256 public count = 0;
+    mapping(uint256 => address) hatWearers;
 
     function mintTopHat(
-        address,
+        address _target,
         string memory,
         string memory
     ) external returns (uint256 topHatId) {
         topHatId = count;
         count++;
+        hatWearers[topHatId] = _target;
     }
 
     function createHat(
@@ -28,9 +30,26 @@ contract MockHats is IHats {
         count++;
     }
 
-    function mintHat(uint256, address) external pure returns (bool success) {
+    function mintHat(
+        uint256 hatId,
+        address wearer
+    ) external returns (bool success) {
         success = true;
+        hatWearers[hatId] = wearer;
+    }
+
+    function transferHat(uint256 _hatId, address _from, address _to) external {
+        require(
+            hatWearers[_hatId] == _from,
+            "MockHats: Invalid current wearer"
+        );
+        hatWearers[_hatId] = _to;
     }
 
-    function transferHat(uint256, address, address) external {}
+    function isWearerOfHat(
+        address _user,
+        uint256 _hatId
+    ) external view returns (bool isWearer) {
+        isWearer = hatWearers[_hatId] == _user;
+    }
 }

test/DecentHats_0_1_0.test.ts

@@ -506,5 +506,127 @@ describe('DecentHats_0_1_0', () => {
         expect(stream2.endTime).to.equal(currentBlockTimestamp + 1296000);
       });
     });
+
+    describe('Creating a new hat on existing Tree', () => {
+      let createRoleHatPromise: Promise<ethers.ContractTransactionResponse>;
+      const topHatId = 0;
+
+      beforeEach(async () => {
+        await executeSafeTransaction({
+          safe: gnosisSafe,
+          to: decentHatsAddress,
+          transactionData: DecentHats_0_1_0__factory.createInterface().encodeFunctionData(
+            'createAndDeclareTree',
+            [
+              {
+                hatsProtocol: mockHatsAddress,
+                hatsAccountImplementation: mockHatsAccountImplementationAddress,
+                registry: await erc6551Registry.getAddress(),
+                keyValuePairs: await keyValuePairs.getAddress(),
+                topHatDetails: '',
+                topHatImageURI: '',
+                adminHat: {
+                  maxSupply: 1,
+                  details: '',
+                  imageURI: '',
+                  isMutable: false,
+                  wearer: ethers.ZeroAddress,
+                  sablierParams: [],
+                },
+                hats: [],
+              },
+            ],
+          ),
+          signers: [dao],
+        });
+
+        const currentBlockTimestamp = (await hre.ethers.provider.getBlock('latest'))!.timestamp;
+
+        createRoleHatPromise = executeSafeTransaction({
+          safe: gnosisSafe,
+          to: decentHatsAddress,
+          transactionData: DecentHats_0_1_0__factory.createInterface().encodeFunctionData(
+            'createRoleHat',
+            [
+              mockHatsAddress,
+              1,
+              {
+                maxSupply: 1,
+                details: '',
+                imageURI: '',
+                isMutable: true,
+                wearer: '0xdce7ca0555101f97451926944f5ae3b7adb2f5ae',
+                sablierParams: [
+                  {
+                    sablier: mockSablierAddress,
+                    sender: gnosisSafeAddress,
+                    totalAmount: ethers.parseEther('100'),
+                    asset: mockERC20Address,
+                    cancelable: true,
+                    transferable: false,
+                    timestamps: {
+                      start: currentBlockTimestamp,
+                      cliff: currentBlockTimestamp + 86400, // 1 day cliff
+                      end: currentBlockTimestamp + 2592000, // 30 days from now
+                    },
+                    broker: { account: ethers.ZeroAddress, fee: 0 },
+                  },
+                ],
+              },
+              0,
+              '0xdce7ca0555101f97451926944f5ae3b7adb2f5ae',
+              await erc6551Registry.getAddress(),
+              mockHatsAccountImplementationAddress,
+              '0x5d0e6ce4fd951366cc55da93f6e79d8b81483109d79676a04bcc2bed6a4b5072',
+            ],
+          ),
+          signers: [dao],
+        });
+      });
+
+      it('Reverts if the top hat is not transferred to the DecentHats module first', async () => {
+        await expect(createRoleHatPromise).to.be.reverted;
+      });
+
+      it('Emits an ExecutionSuccess event', async () => {
+        // First transfer the top hat to the Safe
+        await mockHats.transferHat(topHatId, gnosisSafeAddress, decentHatsAddress);
+        await expect(await createRoleHatPromise).to.emit(gnosisSafe, 'ExecutionSuccess');
+      });
+
+      it('Emits an ExecutionFromModuleSuccess event', async () => {
+        // First transfer the top hat to the Safe
+        await mockHats.transferHat(topHatId, gnosisSafeAddress, decentHatsAddress);
+        await expect(await createRoleHatPromise)
+          .to.emit(gnosisSafe, 'ExecutionFromModuleSuccess')
+          .withArgs(decentHatsAddress);
+      });
+
+      it('Transfers the top hat back to the Safe', async () => {
+        // First transfer the top hat to the Safe
+        await mockHats.transferHat(topHatId, gnosisSafeAddress, decentHatsAddress);
+
+        const isModuleWearerOfTopHat = await mockHats.isWearerOfHat(decentHatsAddress, topHatId);
+        expect(isModuleWearerOfTopHat).to.equal(true);
+
+        await createRoleHatPromise;
+
+        const isSafeWearerOfTopHat = await mockHats.isWearerOfHat(gnosisSafeAddress, topHatId);
+        expect(isSafeWearerOfTopHat).to.equal(true);
+      });
+
+      it('Actually creates the new hat', async () => {
+        // First transfer the top hat to the Safe
+        await mockHats.transferHat(topHatId, gnosisSafeAddress, decentHatsAddress);
+
+        const hatsCountBeforeCreate = await mockHats.count();
+        expect(hatsCountBeforeCreate).to.equal(2); // Top hat + admin hat
+
+        await createRoleHatPromise;
+
+        const newHatId = await mockHats.count();
+        expect(newHatId).to.equal(3); // + newly created hat
+      });
+    });
   });
 });