Adding new tests (WIP)

README.md

@@ -143,7 +143,7 @@ function grantWithCondition(
 );
 ```
 
-See the `ExecuteSelectorCondition` contract. It restricts what the [StdMemberAddHelper](#main-member-add-helper) can execute on the DAO.
+See the `ExecuteSelectorCondition` contract. It restricts what the [StdMemberAddHelper](#standard-member-add-helper) can execute on the DAO.
 
 [Learn more about OSx permissions](https://devs.aragon.org/docs/osx/how-it-works/core/permissions/)
 
@@ -173,10 +173,10 @@ Spaces:
 
 Governance settings:
 
-- `UPDATE_VOTING_SETTINGS_PERMISSION_ID` is required to change the settings of the [StdGovernancePlugin](#main-voting-plugin)
-- `UPDATE_ADDRESSES_PERMISSION_ID` is required to add or remove members or editors on the [StdGovernancePlugin](#main-voting-plugin)
+- `UPDATE_VOTING_SETTINGS_PERMISSION_ID` is required to change the settings of the [StdGovernancePlugin](#standard-governance-plugin)
+- `UPDATE_ADDRESSES_PERMISSION_ID` is required to add or remove members or editors on the [StdGovernancePlugin](#standard-governance-plugin)
   - Typically called by the DAO via proposal
-- `UPDATE_MULTISIG_SETTINGS_PERMISSION_ID` is required to change the settings of the [StdMemberAddHelper](#main-member-add-helper)
+- `UPDATE_MULTISIG_SETTINGS_PERMISSION_ID` is required to change the settings of the [StdMemberAddHelper](#standard-member-add-helper)
   - Typically called by the DAO via proposal
 
 Permission management:
@@ -221,7 +221,7 @@ It uses the generated typechain artifacts, which contain the interfaces for the
 
 ## Adding members and editors
 
-On Spaces with the standard governance, a [StdMemberAddHelper](#main-member-add-helper) and a [StdGovernancePlugin](#main-voting-plugin) will be installed.
+On Spaces with the standard governance, a [StdMemberAddHelper](#standard-member-add-helper) and a [StdGovernancePlugin](#standard-governance-plugin) will be installed.
 
 ### Members
 
@@ -302,7 +302,7 @@ event SubspaceRemoved(address dao, address subspaceDao);
 - The DAO can upgrade the plugin
 - See [Plugin upgrader](#plugin-upgrader) (optional)
 
-### Main Member Add Helper
+### Standard Member Add Helper
 
 Provides a simple way for any address to request membership on a space. It is a adapted version of Aragon's [Multisig plugin](https://github.com/aragon/osx/blob/develop/packages/contracts/src/plugins/governance/multisig/Multisig.sol). It creates a proposal to `addMember()` on the standard governance plugin and Editors can approve or reject it. Once approved, the member create proposals on the standard governance plugin.
 
@@ -396,7 +396,7 @@ event ProposalExecuted(uint256 indexed proposalId);
 
 ### Standard Governance plugin
 
-It's the main governance plugin for standard spaces, where all proposals are voted by editors. It is a adapted version of Aragon's [AddresslistVoting plugin](https://github.com/aragon/osx/blob/develop/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol). Only members (or editors) can create proposals and they can only be executed after a qualified majority has voted for it.
+It's the governance plugin for standard spaces, where all proposals are voted by editors. It is a adapted version of Aragon's [AddresslistVoting plugin](https://github.com/aragon/osx/blob/develop/packages/contracts/src/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol). Only members (or editors) can create proposals and they can only be executed after a qualified majority has voted for it.
 
 It acts as the source of truth about who is an editor, on Spaces with standard governance.
 

packages/contracts-ethers/README.md

@@ -16,7 +16,7 @@ import {
 } from "<package-name>";
 
 // Use it
-const mainVotingPluinInstance = StdGovernancePluginFactory__factory.connect(...);
+const stdGovernancePluinInstance = StdGovernancePluginFactory__factory.connect(...);
 ```
 
 ## Development

packages/contracts/src/personal/PersonalAdminPlugin.sol

@@ -163,6 +163,10 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors,
     /// @param _newMember The address to grant member permission to
     /// @dev Called by the DAO via the PersonalMemberAddHelper. Not by members or editors.
     function addMember(address _newMember) public auth(ADD_MEMBER_PERMISSION_ID) {
+        if (dao().hasPermission(address(this), _newMember, MEMBER_PERMISSION_ID, bytes(""))) {
+            revert AlreadyAMember(_newMember);
+        }
+
         IDAO.Action[] memory _actions = new IDAO.Action[](1);
         _actions[0].to = address(dao());
         _actions[0].data = abi.encodeCall(
@@ -258,15 +262,15 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors,
         emit EditorRemoved(address(dao()), _editor);
     }
 
-    /// @notice Creates a proposal on PersonalMemberAddHelper to add a new member.
+    /// @notice Creates a proposal on the PersonalMemberAddHelper to add a new member. If approved, `addMember` will be called back.
     /// @param _metadataContentUri The metadata of the proposal.
     /// @param _proposedMember The address of the member who may eveutnally be added.
     /// @return proposalId NOTE: The proposal ID will belong to the helper, not to this contract.
     function proposeAddMember(
         bytes calldata _metadataContentUri,
         address _proposedMember
     ) public returns (uint256 proposalId) {
-        if (isMember(_proposedMember)) {
+        if (dao().hasPermission(address(this), _proposedMember, MEMBER_PERMISSION_ID, bytes(""))) {
             revert AlreadyAMember(_proposedMember);
         }
 

packages/contracts/src/personal/PersonalMemberAddHelper.sol

@@ -216,6 +216,7 @@ contract PersonalMemberAddHelper is PluginCloneable, ProposalUpgradeable {
 
         emit Approved({proposalId: _proposalId, editor: _approver});
 
+        // Only one approval is required: Execute
         _execute(_proposalId);
     }
 
@@ -287,7 +288,7 @@ contract PersonalMemberAddHelper is PluginCloneable, ProposalUpgradeable {
         Proposal storage proposal_ = proposals[_proposalId];
 
         if (!_isProposalOpen(proposal_)) {
-            // The proposal was executed already
+            // The proposal is executed, unstarted or expired
             return false;
         } else if (!proposal_.destinationPlugin.isEditor(_account)) {
             // The approver has no voting power.

packages/contracts/src/standard/StdGovernancePlugin.sol

@@ -10,11 +10,11 @@ import {MajorityVotingBase} from "./base/MajorityVotingBase.sol";
 import {IMembers} from "../base/IMembers.sol";
 import {IEditors} from "../base/IEditors.sol";
 import {Addresslist} from "./base/Addresslist.sol";
-import {StdMemberAddHelper, MAIN_MEMBER_ADD_INTERFACE_ID} from "./StdMemberAddHelper.sol";
+import {StdMemberAddHelper, STD_MEMBER_ADD_INTERFACE_ID} from "./StdMemberAddHelper.sol";
 import {SpacePlugin} from "../space/SpacePlugin.sol";
 
 // The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract.
-bytes4 constant MAIN_SPACE_VOTING_INTERFACE_ID = StdGovernancePlugin.initialize.selector ^
+bytes4 constant STD_GOVERNANCE_PLUGIN_INTERFACE_ID = StdGovernancePlugin.initialize.selector ^
     StdGovernancePlugin.createProposal.selector ^
     StdGovernancePlugin.proposeEdits.selector ^
     StdGovernancePlugin.proposeAcceptSubspace.selector ^
@@ -114,7 +114,7 @@ contract StdGovernancePlugin is Addresslist, MajorityVotingBase, IEditors, IMemb
         _addAddresses(_initialEditors);
         emit EditorsAdded(_initialEditors);
 
-        if (!_stdMemberAddHelper.supportsInterface(MAIN_MEMBER_ADD_INTERFACE_ID)) {
+        if (!_stdMemberAddHelper.supportsInterface(STD_MEMBER_ADD_INTERFACE_ID)) {
             revert InvalidInterface(address(_stdMemberAddHelper));
         }
         stdMemberAddHelper = _stdMemberAddHelper;
@@ -125,7 +125,7 @@ contract StdGovernancePlugin is Addresslist, MajorityVotingBase, IEditors, IMemb
     /// @return Returns `true` if the interface is supported.
     function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
         return
-            _interfaceId == MAIN_SPACE_VOTING_INTERFACE_ID ||
+            _interfaceId == STD_GOVERNANCE_PLUGIN_INTERFACE_ID ||
             _interfaceId == type(Addresslist).interfaceId ||
             _interfaceId == type(MajorityVotingBase).interfaceId ||
             _interfaceId == type(IMembers).interfaceId ||

packages/contracts/src/standard/StdMemberAddHelper.sol

@@ -11,9 +11,9 @@ import {ProposalUpgradeable} from "@aragon/osx/core/plugin/proposal/ProposalUpgr
 import {Addresslist} from "./base/Addresslist.sol";
 import {IMultisig} from "./base/IMultisig.sol";
 import {IEditors} from "../base/IEditors.sol";
-import {StdGovernancePlugin, MAIN_SPACE_VOTING_INTERFACE_ID} from "./StdGovernancePlugin.sol";
+import {StdGovernancePlugin, STD_GOVERNANCE_PLUGIN_INTERFACE_ID} from "./StdGovernancePlugin.sol";
 
-bytes4 constant MAIN_MEMBER_ADD_INTERFACE_ID = StdMemberAddHelper.initialize.selector ^
+bytes4 constant STD_MEMBER_ADD_INTERFACE_ID = StdMemberAddHelper.initialize.selector ^
     StdMemberAddHelper.updateMultisigSettings.selector ^
     StdMemberAddHelper.proposeAddMember.selector ^
     StdMemberAddHelper.getProposal.selector;
@@ -138,7 +138,7 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade
         bytes4 _interfaceId
     ) public view virtual override(PluginUUPSUpgradeable, ProposalUpgradeable) returns (bool) {
         return
-            _interfaceId == MAIN_MEMBER_ADD_INTERFACE_ID ||
+            _interfaceId == STD_MEMBER_ADD_INTERFACE_ID ||
             _interfaceId == type(IMultisig).interfaceId ||
             super.supportsInterface(_interfaceId);
     }
@@ -163,7 +163,9 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade
     ) public auth(PROPOSER_PERMISSION_ID) returns (uint256 proposalId) {
         // Check that the caller supports the `addMember` function
         if (
-            !StdGovernancePlugin(msg.sender).supportsInterface(MAIN_SPACE_VOTING_INTERFACE_ID) ||
+            !StdGovernancePlugin(msg.sender).supportsInterface(
+                STD_GOVERNANCE_PLUGIN_INTERFACE_ID
+            ) ||
             !StdGovernancePlugin(msg.sender).supportsInterface(type(IEditors).interfaceId) ||
             !StdGovernancePlugin(msg.sender).supportsInterface(type(Addresslist).interfaceId)
         ) {

packages/contracts/test/integration-testing/member-add-condition.ts → packages/contracts/test/integration-testing/execute-selector-condition.ts

@@ -35,7 +35,7 @@ const pluginSettings: MajorityVotingBase.VotingSettingsStruct = {
 };
 const memberAddProposalDuration = 60 * 60 * 24;
 
-describe('Member Add Condition E2E', () => {
+describe('Execute selector condition E2E', () => {
   let deployer: SignerWithAddress;
   let pluginUpgrader: SignerWithAddress;
   let alice: SignerWithAddress;
@@ -46,7 +46,7 @@ describe('Member Add Condition E2E', () => {
 
   let pluginSetupRef: PluginSetupRefStruct;
   let pluginSetup: TestStdGovernanceSetup;
-  let gpsFactory: TestStdGovernanceSetup__factory;
+  let sgsFactory: TestStdGovernanceSetup__factory;
   let stdGovernancePlugin: StdGovernancePlugin;
   // let stdMemberAddHelper: StdMemberAddHelper;
 
@@ -88,8 +88,8 @@ describe('Member Add Condition E2E', () => {
     );
 
     // Deploy PluginSetup build 1
-    gpsFactory = new TestStdGovernanceSetup__factory().connect(deployer);
-    pluginSetup = await gpsFactory.deploy(psp.address);
+    sgsFactory = new TestStdGovernanceSetup__factory().connect(deployer);
+    pluginSetup = await sgsFactory.deploy(psp.address);
 
     // Publish build 1
     tx = await pluginRepo.createVersion(1, pluginSetup.address, '0x00', '0x00');

packages/contracts/test/unit-testing/execute-selector-condition.ts

@@ -20,13 +20,11 @@ import {ethers, network} from 'hardhat';
 const SOME_CONTRACT_ADDRESS = '0x' + '1234567890'.repeat(4);
 const ONE_BYTES32 =
   '0x0000000000000000000000000000000000000000000000000000000000000001';
-const PLUGIN_ADDR_1 = ADDRESS_ONE;
-const PLUGIN_ADDR_2 = ADDRESS_TWO;
 const daoInterface = DAO__factory.createInterface();
 const stdGovernancePluginInterface =
   StdGovernancePlugin__factory.createInterface();
 
-describe('Member Add Condition', function () {
+describe('Execute selector Condition', function () {
   const pspAddress = getPluginSetupProcessorAddress(network.name, true);
 
   let alice: SignerWithAddress;