diff --git a/packages/contracts/src/governance/MainVotingPlugin.sol b/packages/contracts/src/governance/MainVotingPlugin.sol index b6b7865..0d8b63d 100644 --- a/packages/contracts/src/governance/MainVotingPlugin.sol +++ b/packages/contracts/src/governance/MainVotingPlugin.sol @@ -388,16 +388,12 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers /// @notice Creates a proposal to remove an existing member. /// @param _metadata The metadata of the proposal. /// @param _proposedMember The address of the member who may eveutnally be removed. - /// @param _spacePlugin The address of the space plugin where changes will be executed function proposeRemoveMember( bytes calldata _metadata, - address _proposedMember, - address _spacePlugin + address _proposedMember ) public onlyMembers { if (!isEditor(msg.sender)) { revert ProposalCreationForbidden(msg.sender); - } else if (_spacePlugin == address(0)) { - revert EmptyContent(); } else if (!isMember(_proposedMember)) { revert AlreadyNotMember(_proposedMember); } diff --git a/packages/contracts/test/integration-testing/member-access-condition.ts b/packages/contracts/test/integration-testing/member-access-condition.ts index 402fd1f..5ae6f97 100644 --- a/packages/contracts/test/integration-testing/member-access-condition.ts +++ b/packages/contracts/test/integration-testing/member-access-condition.ts @@ -22,6 +22,7 @@ import { ROOT_PERMISSION_ID, UPGRADE_PLUGIN_PERMISSION_ID, ONE_BYTES32, + ADDRESS_ONE, } from '../unit-testing/common'; import { DAO, @@ -193,40 +194,6 @@ describe('Member Access Condition E2E', () => { expect(await mainVotingPlugin.isMember(bob.address)).to.eq(true); }); - it('Executing a proposal to remove membership works', async () => { - await expect(memberAccessPlugin.proposeNewMember('0x', alice.address)).to - .not.be.reverted; - await expect(memberAccessPlugin.proposeRemoveMember('0x', alice.address)).to - .not.be.reverted; - expect(await mainVotingPlugin.isMember(alice.address)).to.eq(false); - - // Valid revoke - const grantAction = { - to: mainVotingPlugin.address, - value: 0, - data: mainVotingInterface.encodeFunctionData('addMember', [bob.address]), - }; - const revokeAction = { - to: mainVotingPlugin.address, - value: 0, - data: mainVotingInterface.encodeFunctionData('removeMember', [ - bob.address, - ]), - }; - - // Via direct create proposal - await expect( - memberAccessPlugin.createArbitraryProposal('0x', [grantAction]) - ).to.not.be.reverted; - expect(await mainVotingPlugin.isMember(bob.address)).to.eq(true); - - await expect( - memberAccessPlugin.createArbitraryProposal('0x', [revokeAction]) - ).to.not.be.reverted; - - expect(await mainVotingPlugin.isMember(bob.address)).to.eq(false); - }); - it('Executing a proposal to do something else reverts', async () => { const validActions = [ { @@ -239,12 +206,19 @@ describe('Member Access Condition E2E', () => { { to: mainVotingPlugin.address, value: 0, - data: mainVotingInterface.encodeFunctionData('removeMember', [ - bob.address, + data: mainVotingInterface.encodeFunctionData('addMember', [ + ADDRESS_ONE, ]), }, ]; const invalidActions = [ + { + to: mainVotingPlugin.address, + value: 0, + data: mainVotingInterface.encodeFunctionData('removeMember', [ + bob.address, + ]), + }, { to: dao.address, value: 0, diff --git a/packages/contracts/test/unit-testing/main-voting-plugin.ts b/packages/contracts/test/unit-testing/main-voting-plugin.ts index 1f58f1d..7bb5113 100644 --- a/packages/contracts/test/unit-testing/main-voting-plugin.ts +++ b/packages/contracts/test/unit-testing/main-voting-plugin.ts @@ -390,38 +390,41 @@ describe('Main Voting Plugin', function () { }); it('isMember() returns true when appropriate', async () => { - expect(await memberAccessPlugin.isMember(ADDRESS_ZERO)).to.eq(false); - expect(await memberAccessPlugin.isMember(ADDRESS_ONE)).to.eq(false); - expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(false); + expect(await mainVotingPlugin.isMember(ADDRESS_ZERO)).to.eq(false); + expect(await mainVotingPlugin.isMember(ADDRESS_ONE)).to.eq(false); + expect(await mainVotingPlugin.isMember(ADDRESS_TWO)).to.eq(false); - expect(await memberAccessPlugin.isMember(alice.address)).to.eq(true); - expect(await memberAccessPlugin.isMember(bob.address)).to.eq(true); + expect(await mainVotingPlugin.isMember(alice.address)).to.eq(true); + expect(await mainVotingPlugin.isMember(bob.address)).to.eq(true); - expect(await memberAccessPlugin.isMember(carol.address)).to.eq(false); + expect(await mainVotingPlugin.isMember(carol.address)).to.eq(false); await memberAccessPlugin.proposeNewMember('0x', carol.address); - expect(await memberAccessPlugin.isMember(carol.address)).to.eq(true); + expect(await mainVotingPlugin.isMember(carol.address)).to.eq(true); - await memberAccessPlugin.proposeRemoveMember('0x', carol.address); - expect(await memberAccessPlugin.isMember(carol.address)).to.eq(false); + await mainVotingPlugin + .connect(bob) + .proposeRemoveMember('0x', carol.address); + await mainVotingPlugin.vote(0, VoteOption.Yes, true); + expect(await mainVotingPlugin.isMember(carol.address)).to.eq(false); await makeEditor(carol.address); - expect(await memberAccessPlugin.isMember(carol.address)).to.eq(true); + expect(await mainVotingPlugin.isMember(carol.address)).to.eq(true); }); it('isEditor() returns true when appropriate', async () => { - expect(await memberAccessPlugin.isEditor(ADDRESS_ZERO)).to.eq(false); - expect(await memberAccessPlugin.isEditor(ADDRESS_ONE)).to.eq(false); - expect(await memberAccessPlugin.isEditor(ADDRESS_TWO)).to.eq(false); + expect(await mainVotingPlugin.isEditor(ADDRESS_ZERO)).to.eq(false); + expect(await mainVotingPlugin.isEditor(ADDRESS_ONE)).to.eq(false); + expect(await mainVotingPlugin.isEditor(ADDRESS_TWO)).to.eq(false); - expect(await memberAccessPlugin.isEditor(alice.address)).to.eq(true); - expect(await memberAccessPlugin.isEditor(bob.address)).to.eq(false); - expect(await memberAccessPlugin.isEditor(carol.address)).to.eq(false); + expect(await mainVotingPlugin.isEditor(alice.address)).to.eq(true); + expect(await mainVotingPlugin.isEditor(bob.address)).to.eq(false); + expect(await mainVotingPlugin.isEditor(carol.address)).to.eq(false); await makeEditor(carol.address); - expect(await memberAccessPlugin.isEditor(carol.address)).to.eq(true); + expect(await mainVotingPlugin.isEditor(carol.address)).to.eq(true); }); }); diff --git a/packages/contracts/test/unit-testing/member-access-plugin.ts b/packages/contracts/test/unit-testing/member-access-plugin.ts index 5e93706..69e3e0e 100644 --- a/packages/contracts/test/unit-testing/member-access-plugin.ts +++ b/packages/contracts/test/unit-testing/member-access-plugin.ts @@ -293,7 +293,10 @@ describe('Member Access Plugin', function () { await memberAccessPlugin.proposeNewMember('0x', carol.address); expect(await memberAccessPlugin.isMember(carol.address)).to.eq(true); - await memberAccessPlugin.proposeRemoveMember('0x', carol.address); + await mainVotingPlugin + .connect(bob) + .proposeRemoveMember('0x', carol.address); + await mainVotingPlugin.vote(0, VoteOption.Yes, true); expect(await memberAccessPlugin.isMember(carol.address)).to.eq(false); await proposeNewEditor(carol.address); @@ -435,17 +438,6 @@ describe('Member Access Plugin', function () { // Now Carol is a member expect(await memberAccessPlugin.isMember(carol.address)).to.eq(true); - - // Undo - await expect( - memberAccessPlugin.proposeRemoveMember( - toUtf8Bytes('ipfs://1234'), - carol.address - ) - ).to.not.be.reverted; - - // Carol is no longer a member - expect(await memberAccessPlugin.isMember(carol.address)).to.eq(false); }); it("Proposals created by a non-editor need an editor's approval", async () => { @@ -570,92 +562,6 @@ describe('Member Access Plugin', function () { expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(true); }); - it('Only editors can approve removing members', async () => { - expect(await mainVotingPlugin.addresslistLength()).to.eq(3); - { - pid = await memberAccessPlugin.proposalCount(); - await memberAccessPlugin.proposeNewMember('0x', dave.address); - await memberAccessPlugin.proposeNewMember('0x', ADDRESS_ONE); - await memberAccessPlugin.proposeNewMember('0x', ADDRESS_TWO); - - await memberAccessPlugin.connect(bob).approve(pid); - await memberAccessPlugin.connect(bob).approve(pid.add(1)); - await memberAccessPlugin.connect(bob).approve(pid.add(2)); - } - - // Requesting membership for Dave - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), dave.address) - ).to.not.be.reverted; - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Dave cannot approve (fail) - await expect(memberAccessPlugin.connect(dave).approve(pid)).to.be - .reverted; - - // Dave remains as a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Approve it (Alice) - await expect(memberAccessPlugin.connect(alice).approve(pid)).to.not.be - .reverted; - - // Dave is no longer a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(false); - - // Now requesting for 0x1 - expect(await memberAccessPlugin.isMember(ADDRESS_ONE)).to.eq(true); - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), ADDRESS_ONE) - ).to.not.be.reverted; - expect(await memberAccessPlugin.isMember(ADDRESS_ONE)).to.eq(true); - - // Dave cannot approve (fail) - await expect(memberAccessPlugin.connect(dave).approve(pid)).to.be - .reverted; - - // ADDRESS_ONE remains as a member - expect(await memberAccessPlugin.isMember(ADDRESS_ONE)).to.eq(true); - - // Approve it (Bob) - await expect(memberAccessPlugin.connect(bob).approve(pid)).to.not.be - .reverted; - - // ADDRESS_ONE is no longer a member - expect(await memberAccessPlugin.isMember(ADDRESS_ONE)).to.eq(false); - - // Now requesting for 0x2 - expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(true); - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), ADDRESS_TWO) - ).to.not.be.reverted; - expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(true); - - // Dave cannot approve (fail) - await expect(memberAccessPlugin.connect(dave).approve(pid)).to.be - .reverted; - - // ADDRESS_TWO remains as a member - expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(true); - - // Approve it (Carol) - await expect(memberAccessPlugin.connect(carol).approve(pid)).to.not.be - .reverted; - - // ADDRESS_TWO is no longer a member - expect(await memberAccessPlugin.isMember(ADDRESS_TWO)).to.eq(false); - }); - it('Proposals should be unsettled after created', async () => { expect(await mainVotingPlugin.addresslistLength()).to.eq(3); @@ -736,42 +642,6 @@ describe('Member Access Plugin', function () { expect((await memberAccessPlugin.getProposal(pid)).executed).to.eq(false); }); - it('Only editors can reject membership removal proposals', async () => { - expect(await mainVotingPlugin.addresslistLength()).to.eq(3); - pid = await memberAccessPlugin.proposalCount(); - await memberAccessPlugin.proposeNewMember('0x', dave.address); - await memberAccessPlugin.connect(bob).approve(pid); - - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), dave.address) - ).to.not.be.reverted; - - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Reject it (Dave) => fail - await expect(memberAccessPlugin.connect(dave).reject(pid)).to.be.reverted; - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Reject it (Bob) => success - await expect(memberAccessPlugin.connect(bob).reject(pid)).to.not.be - .reverted; - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Try to approve it (bob) => fail - await expect(memberAccessPlugin.connect(bob).approve(pid)).to.be.reverted; - - expect((await memberAccessPlugin.getProposal(pid)).executed).to.eq(false); - }); - it("Proposals created by a non-editor need an editor's approval", async () => { expect(await mainVotingPlugin.addresslistLength()).to.eq(3); expect(await memberAccessPlugin.isMember(dave.address)).to.eq(false); @@ -849,39 +719,6 @@ describe('Member Access Plugin', function () { // Now Dave is a member expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Alice proposes aremoving a member - - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(alice) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), dave.address) - ).to.not.be.reverted; - - proposal = await memberAccessPlugin.getProposal(pid); - expect(proposal.executed).to.eq(false); - - // Approve it (Alice) => fail - await expect(memberAccessPlugin.connect(alice).approve(pid)).to.be - .reverted; - - // Approve it (Dave) => fail - await expect(memberAccessPlugin.connect(dave).approve(pid)).to.be - .reverted; - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Approve it (Bob) => succeed - await expect(memberAccessPlugin.connect(bob).approve(pid)).to.not.be - .reverted; - - proposal = await memberAccessPlugin.getProposal(pid); - expect(proposal.executed).to.eq(true); - - // Now Dave is a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(false); }); it('Memberships are rejected when the first non-proposer editor rejects', async () => { @@ -916,49 +753,6 @@ describe('Member Access Plugin', function () { // Dave is still not a member expect(await memberAccessPlugin.isMember(dave.address)).to.eq(false); - - // Alice proposes removing a member - - pid = await memberAccessPlugin.proposalCount(); - await memberAccessPlugin.proposeNewMember('0x', dave.address); - await memberAccessPlugin.connect(bob).approve(pid); - - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(alice) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), dave.address) - ).to.not.be.reverted; - - expect((await memberAccessPlugin.getProposal(pid)).executed).to.eq(false); - - // Reject it (Alice) => can't change - await expect(memberAccessPlugin.connect(alice).reject(pid)).to.be - .reverted; - - // Reject it (Dave) => fail - await expect(memberAccessPlugin.connect(dave).reject(pid)).to.be.reverted; - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Reject it (Bob) => succeed - await expect(memberAccessPlugin.connect(bob).reject(pid)).to.not.be - .reverted; - - expect((await memberAccessPlugin.getProposal(pid)).executed).to.eq(false); - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); - - // Reject it (Carol) => succeed - await expect(memberAccessPlugin.connect(carol).reject(pid)).to.be - .reverted; - - expect((await memberAccessPlugin.getProposal(pid)).executed).to.eq(false); - - // Still a member - expect(await memberAccessPlugin.isMember(dave.address)).to.eq(true); }); }); @@ -985,31 +779,12 @@ describe('Member Access Plugin', function () { ); }); - it('proposeRemoveMember should generate the right action list', async () => { - pid = await memberAccessPlugin.proposalCount(); - await expect( - memberAccessPlugin - .connect(bob) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), bob.address) - ).to.not.be.reverted; - - const proposal = await memberAccessPlugin.getProposal(pid); - expect(proposal.actions.length).to.eq(1); - expect(proposal.actions[0].to).to.eq(mainVotingPlugin.address); - expect(proposal.actions[0].value).to.eq(0); - expect(proposal.actions[0].data).to.eq( - mainVotingPluginInterface.encodeFunctionData('removeMember', [ - bob.address, - ]) - ); - }); - it('Attempting to approve twice fails', async () => { pid = await memberAccessPlugin.proposalCount(); await expect( memberAccessPlugin .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), bob.address) + .proposeNewMember(toUtf8Bytes('ipfs://1234'), bob.address) ).to.not.be.reverted; await expect(memberAccessPlugin.approve(pid)).to.not.be.reverted; @@ -1021,7 +796,7 @@ describe('Member Access Plugin', function () { await expect( memberAccessPlugin .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), bob.address) + .proposeNewMember(toUtf8Bytes('ipfs://1234'), bob.address) ).to.not.be.reverted; await expect(memberAccessPlugin.reject(pid)).to.not.be.reverted; @@ -1048,32 +823,12 @@ describe('Member Access Plugin', function () { ).to.be.reverted; }); - it('Attempting to propose removing a non-member fails', async () => { - await expect( - memberAccessPlugin - .connect(carol) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), carol.address) - ).to.be.reverted; - - await expect( - memberAccessPlugin - .connect(bob) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), ADDRESS_ONE) - ).to.be.reverted; - - await expect( - memberAccessPlugin - .connect(alice) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), ADDRESS_TWO) - ).to.be.reverted; - }); - it('Rejected proposals cannot be approved', async () => { pid = await memberAccessPlugin.proposalCount(); await expect( memberAccessPlugin .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), bob.address) + .proposeNewMember(toUtf8Bytes('ipfs://1234'), bob.address) ).to.not.be.reverted; await expect(memberAccessPlugin.reject(pid)).to.not.be.reverted; @@ -1085,7 +840,7 @@ describe('Member Access Plugin', function () { await expect( memberAccessPlugin .connect(dave) - .proposeRemoveMember(toUtf8Bytes('ipfs://1234'), bob.address) + .proposeNewMember(toUtf8Bytes('ipfs://1234'), bob.address) ).to.not.be.reverted; await expect(memberAccessPlugin.reject(pid)).to.not.be.reverted;