fix: remove unnecessary view functions for MultiOwnerPlugin (#68)

src/plugins/owner/IMultiOwnerPlugin.sol

@@ -28,19 +28,6 @@ interface IMultiOwnerPlugin {
     /// @param ownersToRemove The address array of owners to be removed.
     function updateOwners(address[] memory ownersToAdd, address[] memory ownersToRemove) external;
 
-    /// @notice Check if an address is an owner of the current account.
-    /// @dev This function is installed on the account as part of plugin installation, and should
-    /// only be called from an account.
-    /// @param ownerToCheck The owner to check if it is an owner of the current account.
-    /// @return True if the address is an owner of the account.
-    function isOwner(address ownerToCheck) external view returns (bool);
-
-    /// @notice Get the owners of the current account.
-    /// @dev This function is installed on the account as part of plugin installation, and should
-    /// only be called from an account.
-    /// @return The addresses of the owners of the account.
-    function owners() external view returns (address[] memory);
-
     /// @notice Gets the EIP712 domain
     /// @dev This implementation is different from typical 712 via its use of msg.sender instead. As such, it
     /// should only be called from the SCAs that has installed this. See ERC-5267.

src/plugins/owner/MultiOwnerPlugin.sol

@@ -98,16 +98,6 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
     // ┃  Execution view functions   ┃
     // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
 
-    /// @inheritdoc IMultiOwnerPlugin
-    function isOwner(address ownerToCheck) external view returns (bool) {
-        return isOwnerOf(msg.sender, ownerToCheck);
-    }
-
-    /// @inheritdoc IMultiOwnerPlugin
-    function owners() external view returns (address[] memory) {
-        return ownersOf(msg.sender);
-    }
-
     /// @inheritdoc IMultiOwnerPlugin
     function eip712Domain()
         public
@@ -253,12 +243,10 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
     function pluginManifest() external pure override returns (PluginManifest memory) {
         PluginManifest memory manifest;
 
-        manifest.executionFunctions = new bytes4[](5);
+        manifest.executionFunctions = new bytes4[](3);
         manifest.executionFunctions[0] = this.updateOwners.selector;
-        manifest.executionFunctions[1] = this.owners.selector;
-        manifest.executionFunctions[2] = this.isOwner.selector;
-        manifest.executionFunctions[3] = this.eip712Domain.selector;
-        manifest.executionFunctions[4] = this.isValidSignature.selector;
+        manifest.executionFunctions[1] = this.eip712Domain.selector;
+        manifest.executionFunctions[2] = this.isValidSignature.selector;
 
         ManifestFunction memory ownerUserOpValidationFunction = ManifestFunction({
             functionType: ManifestAssociatedFunctionType.SELF,
@@ -305,7 +293,7 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
         });
 
         // Update Modular Account's native functions to use runtimeValidationFunction provided by this plugin
-        manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](10);
+        manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](8);
         manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({
             executionSelector: this.updateOwners.selector,
             associatedFunction: ownerOrSelfRuntimeValidationFunction
@@ -335,14 +323,6 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
             associatedFunction: alwaysAllowFunction
         });
         manifest.runtimeValidationFunctions[7] = ManifestAssociatedFunction({
-            executionSelector: this.isOwner.selector,
-            associatedFunction: alwaysAllowFunction
-        });
-        manifest.runtimeValidationFunctions[8] = ManifestAssociatedFunction({
-            executionSelector: this.owners.selector,
-            associatedFunction: alwaysAllowFunction
-        });
-        manifest.runtimeValidationFunctions[9] = ManifestAssociatedFunction({
             executionSelector: this.eip712Domain.selector,
             associatedFunction: alwaysAllowFunction
         });

test/plugin/owner/MultiOwnerPlugin.t.sol

@@ -58,18 +58,17 @@ contract MultiOwnerPluginTest is Test {
 
     function test_pluginManifest() public {
         PluginManifest memory manifest = plugin.pluginManifest();
-        // 5 execution functions
-        assertEq(5, manifest.executionFunctions.length);
+        // 3 execution functions
+        assertEq(3, manifest.executionFunctions.length);
         // 5 native + 1 plugin exec func
         assertEq(6, manifest.userOpValidationFunctions.length);
-        // 5 native + 1 plugin exec func + 4 plugin view func
-        assertEq(10, manifest.runtimeValidationFunctions.length);
+        // 5 native + 1 plugin exec func + 2 plugin view func
+        assertEq(8, manifest.runtimeValidationFunctions.length);
     }
 
     function test_onUninstall_success() public {
         plugin.onUninstall(abi.encode(""));
         address[] memory returnedOwners = plugin.ownersOf(accountA);
-        assertEq(returnedOwners, plugin.owners());
         assertEq(0, returnedOwners.length);
     }
 
@@ -80,7 +79,6 @@ contract MultiOwnerPluginTest is Test {
         vm.startPrank(address(contractOwner));
         plugin.onInstall(abi.encode(owners));
         address[] memory returnedOwners = plugin.ownersOf(address(contractOwner));
-        assertEq(returnedOwners, plugin.owners());
         assertEq(returnedOwners.length, 1);
         assertEq(returnedOwners[0], owner1);
         vm.stopPrank();
@@ -89,8 +87,6 @@ contract MultiOwnerPluginTest is Test {
     function test_eip712Domain() public {
         assertEq(true, plugin.isOwnerOf(accountA, owner2));
         assertEq(false, plugin.isOwnerOf(accountA, address(contractOwner)));
-        assertEq(true, plugin.isOwner(owner2));
-        assertEq(false, plugin.isOwner(address(contractOwner)));
 
         (
             bytes1 fields,
@@ -133,7 +129,6 @@ contract MultiOwnerPluginTest is Test {
 
     function test_updateOwners_success() public {
         (address[] memory owners) = plugin.ownersOf(accountA);
-        assertEq(owners, plugin.owners());
         assertEq(Utils.reverseAddressArray(ownerArray), owners);
 
         // remove should also work
@@ -144,7 +139,6 @@ contract MultiOwnerPluginTest is Test {
         plugin.updateOwners(new address[](0), ownersToRemove);
 
         (address[] memory newOwnerList) = plugin.ownersOf(accountA);
-        assertEq(newOwnerList, plugin.owners());
         assertEq(newOwnerList.length, 1);
         assertEq(newOwnerList[0], owner3);
     }
@@ -168,8 +162,6 @@ contract MultiOwnerPluginTest is Test {
         address[] memory ownersToAdd = new address[](1);
         ownersToAdd[0] = signer;
 
-        assertEq(plugin.isOwnerOf(accountA, signer), plugin.isOwner(signer));
-
         if (!plugin.isOwnerOf(accountA, signer)) {
             // sig check should fail
             assertEq(bytes4(0xFFFFFFFF), plugin.isValidSignature(digest, abi.encodePacked(r, s, v)));
@@ -217,7 +209,7 @@ contract MultiOwnerPluginTest is Test {
     }
 
     function test_multiOwnerPlugin_sentinelIsNotOwner() public {
-        assertEq(false, plugin.isOwner(address(1)));
+        assertFalse(plugin.isOwnerOf(accountA, address(1)));
     }
 
     function testFuzz_userOpValidationFunction_ContractOwner(UserOperation memory userOp) public {
@@ -277,8 +269,6 @@ contract MultiOwnerPluginTest is Test {
         address[] memory ownersToAdd = new address[](1);
         ownersToAdd[0] = signer;
 
-        assertEq(plugin.isOwnerOf(accountA, signer), plugin.isOwner(signer));
-
         // Only check that the signature should fail if the signer is not already an owner
         if (!plugin.isOwnerOf(accountA, signer)) {
             // should fail without owner access

test/plugin/owner/MultiOwnerPluginIntegration.t.sol

@@ -76,9 +76,9 @@ contract MultiOwnerPluginIntegration is Test {
     }
 
     function test_ownerPlugin_successInstallation() public {
-        assertTrue(IMultiOwnerPlugin(address(account)).isOwner(owner1));
-        assertTrue(IMultiOwnerPlugin(address(account)).isOwner(owner2));
-        assertEq(Utils.reverseAddressArray(owners), IMultiOwnerPlugin(address(account)).owners());
+        assertTrue(multiOwnerPlugin.isOwnerOf(address(account), owner1));
+        assertTrue(multiOwnerPlugin.isOwnerOf(address(account), owner2));
+        assertEq(Utils.reverseAddressArray(owners), multiOwnerPlugin.ownersOf(address(account)));
     }
 
     function test_runtimeValidation_alwaysAllow_isValidSignature() public {

test/upgrade/MSCAToMSCA.t.sol

@@ -66,7 +66,9 @@ contract MSCAToMSCATest is Test {
         msca.upgradeToAndCall(mscaImpl2, "");
 
         // verify account storage is the same
-        (, bytes memory returnData) = address(msca).call(abi.encodeWithSelector(MultiOwnerPlugin.owners.selector));
+        (, bytes memory returnData) = address(multiOwnerPlugin).call(
+            abi.encodeWithSelector(MultiOwnerPlugin.ownersOf.selector, address(msca))
+        );
         address[] memory returnedOwners = abi.decode(returnData, (address[]));
         assertEq(Utils.reverseAddressArray(returnedOwners), owners);
         assertEq(token1.balanceOf(address(msca)), 1 ether);