refactor: remove redundant factory check and correct return type for SMA

erc6900 · Aug 15, 2024 · 6715717 · 6715717
1 parent 4324a3e
commit 6715717
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 10 deletions.
diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol
@@ -20,6 +20,8 @@ contract AccountFactory is Ownable {
     IEntryPoint public immutable ENTRY_POINT;
     address public immutable SINGLE_SIGNER_VALIDATION_MODULE;
 
+    error SemiModularAccountAddressMismatch(address expected, address returned);
+
     constructor(
         IEntryPoint _entryPoint,
         UpgradeableModularAccount _accountImpl,
@@ -66,21 +68,23 @@ contract AccountFactory is Ownable {
         return UpgradeableModularAccount(payable(addr));
     }
 
-    function createSemiModularAccount(address owner, uint256 salt) external returns (UpgradeableModularAccount) {
+    function createSemiModularAccount(address owner, uint256 salt) external returns (SemiModularAccount) {
         // both module address and entityId for fallback validations are hardcoded at the maximum value.
         bytes32 fullSalt = getSalt(owner, salt, type(uint32).max);
 
         bytes memory immutables = _getImmutableArgs(owner);
 
         address addr = _getAddressFallbackSigner(immutables, fullSalt);
 
-        // short circuit if exists
-        if (addr.code.length == 0) {
-            // not necessary to check return addr since next call will fail if so
+        // LibClone short-circuits if it's already deployed.
+        (, address instance) =
             LibClone.createDeterministicERC1967(address(SEMI_MODULAR_ACCOUNT_IMPL), immutables, fullSalt);
+
+        if (instance != addr) {
+            revert SemiModularAccountAddressMismatch(addr, instance);
         }
 
-        return UpgradeableModularAccount(payable(addr));
+        return SemiModularAccount(payable(addr));
     }
 
     function addStake(uint32 unstakeDelay) external payable onlyOwner {

diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol
@@ -25,6 +25,8 @@ contract SingleSignerFactoryFixture is OptimizedTest {
 
     address public self;
 
+    error SemiModularAccountAddressMismatch(address expected, address returned);
+
     constructor(IEntryPoint _entryPoint, SingleSignerValidationModule _singleSignerValidationModule) {
         entryPoint = _entryPoint;
 
@@ -81,12 +83,12 @@ contract SingleSignerFactoryFixture is OptimizedTest {
 
         address addr = _getAddressFallbackSigner(immutables, fullSalt);
 
-        // short circuit if exists
-        if (addr.code.length == 0) {
-            // not necessary to check return addr since next call will fail if so
-            // new ERC1967Proxy{salt: getSalt(owner, salt, type(uint32).max)}(address(ACCOUNT_IMPL), "");
-            // UpgradeableModularAccount(payable(addr)).initialize(owner);
+        // LibClone short-circuits if it's already deployed.
+        (, address instance) =
             LibClone.createDeterministicERC1967(address(accountImplementation), immutables, fullSalt);
+
+        if (instance != addr) {
+            revert SemiModularAccountAddressMismatch(addr, instance);
         }
 
         return UpgradeableModularAccount(payable(addr));