F/OS-1224 Multisig approvers not appearing in past proposals

packages/subgraph/schema.graphql

@@ -888,14 +888,23 @@ type MultisigPlugin implements IPlugin @entity {
 }
 
 type MultisigApprover @entity {
+  """
+  Current Approver of the multisig. If the approver is removed from the multisig,
+  the entity is not deleted and instead the isActive flag is set to false.
+  """
   id: ID! # plugin_address + member_address
-  address: String # address as string to facilitate filtering by address on the UI
-  proposals: [MultisigProposalApprover!]! @derivedFrom(field: "approver")
+  address: Bytes!
+  approvals: [MultisigProposalApproval!]! @derivedFrom(field: "approver")
   plugin: MultisigPlugin!
+  isActive: Boolean!
 }
 
-type MultisigProposalApprover @entity(immutable: true) {
-  "ApproverProposal for Many-to-Many"
+type MultisigProposalApproval @entity(immutable: true) {
+  """
+  An approval of an specific multisig proposal
+  This entity works as a join table between MultisigProposal and MultisigApprover
+  and also stores the timestamp of the approval
+  """
   id: ID! # approver + proposal
   approver: MultisigApprover!
   proposal: MultisigProposal!
@@ -918,12 +927,12 @@ type MultisigProposal implements IProposal @entity {
   creationBlockNumber: BigInt!
   snapshotBlock: BigInt!
   minApprovals: Int!
-  approvals: Int
+  approvalCount: Int
   approvalReached: Boolean!
   isSignaling: Boolean!
   executed: Boolean!
   executionDate: BigInt
   executionBlockNumber: BigInt
   executionTxHash: Bytes
-  approvers: [MultisigProposalApprover!]! @derivedFrom(field: "proposal")
+  approvals: [MultisigProposalApproval!]! @derivedFrom(field: "proposal")
 }

packages/subgraph/src/packages/multisig/multisig.ts

@@ -3,7 +3,7 @@ import {
   MultisigPlugin,
   MultisigProposal,
   MultisigApprover,
-  MultisigProposalApprover,
+  MultisigProposalApproval,
 } from '../../../generated/schema';
 import {
   ProposalCreated,
@@ -20,7 +20,7 @@ import {
   generatePluginEntityId,
   generateProposalEntityId,
 } from '@aragon/osx-commons-subgraph';
-import {dataSource, store} from '@graphprotocol/graph-ts';
+import {dataSource} from '@graphprotocol/graph-ts';
 
 export function handleProposalCreated(event: ProposalCreated): void {
   let context = dataSource.context();
@@ -60,7 +60,7 @@ export function _handleProposalCreated(
 
   if (!proposal.reverted) {
     proposalEntity.executed = proposal.value.value0;
-    proposalEntity.approvals = proposal.value.value1;
+    proposalEntity.approvalCount = proposal.value.value1;
 
     // ProposalParameters
     let parameters = proposal.value.value2;
@@ -102,24 +102,22 @@ export function _handleProposalCreated(
 export function handleApproved(event: Approved): void {
   let memberAddress = event.params.approver;
   let pluginAddress = event.address;
-  let memberEntityId = generateMemberEntityId(pluginAddress, memberAddress);
+  let approverEntityId = generateMemberEntityId(pluginAddress, memberAddress);
   let pluginProposalId = event.params.proposalId;
   let proposalEntityId = generateProposalEntityId(
     event.address,
     pluginProposalId
   );
   let approverProposalId = generateVoterEntityId(
-    memberEntityId,
+    approverEntityId,
     proposalEntityId
   );
 
-  let approverProposalEntity =
-    MultisigProposalApprover.load(approverProposalId);
-  if (!approverProposalEntity) {
-    approverProposalEntity = new MultisigProposalApprover(approverProposalId);
-    approverProposalEntity.approver = memberEntityId;
-    approverProposalEntity.proposal = proposalEntityId;
-  }
+  // No need for checking if approval exists
+  // a proposal cannot be approved twice by the same approver
+  let approverProposalEntity = new MultisigProposalApproval(approverProposalId);
+  approverProposalEntity.approver = approverEntityId;
+  approverProposalEntity.proposal = proposalEntityId;
   approverProposalEntity.createdAt = event.block.timestamp;
   approverProposalEntity.save();
 
@@ -131,7 +129,7 @@ export function handleApproved(event: Approved): void {
 
     if (!proposal.reverted) {
       let approvals = proposal.value.value1;
-      proposalEntity.approvals = approvals;
+      proposalEntity.approvalCount = approvals;
 
       // calculate if proposal is executable
       let minApprovalsStruct = proposal.value.value2;
@@ -171,15 +169,17 @@ export function handleMembersAdded(event: MembersAdded): void {
   for (let index = 0; index < members.length; index++) {
     const memberAddress = members[index];
     const pluginEntityId = generatePluginEntityId(event.address);
-    const memberEntityId = [pluginEntityId, memberAddress.toHexString()].join(
-      '_'
+    const approverEntityId = generateMemberEntityId(
+      event.address,
+      memberAddress
     );
 
-    let approverEntity = MultisigApprover.load(memberEntityId);
+    let approverEntity = MultisigApprover.load(approverEntityId);
     if (!approverEntity) {
-      approverEntity = new MultisigApprover(memberEntityId);
-      approverEntity.address = memberAddress.toHexString();
+      approverEntity = new MultisigApprover(approverEntityId);
+      approverEntity.address = memberAddress;
       approverEntity.plugin = pluginEntityId;
+      approverEntity.isActive = true;
       approverEntity.save();
     }
   }
@@ -189,14 +189,15 @@ export function handleMembersRemoved(event: MembersRemoved): void {
   const members = event.params.members;
   for (let index = 0; index < members.length; index++) {
     const memberAddress = members[index];
-    const pluginEntityId = generatePluginEntityId(event.address);
-    const memberEntityId = [pluginEntityId, memberAddress.toHexString()].join(
-      '_'
+    const approverEntityId = generateMemberEntityId(
+      event.address,
+      memberAddress
     );
 
-    const approverEntity = MultisigApprover.load(memberEntityId);
+    const approverEntity = MultisigApprover.load(approverEntityId);
     if (approverEntity) {
-      store.remove('MultisigApprover', memberEntityId);
+      approverEntity.isActive = false;
+      approverEntity.save();
     }
   }
 }

packages/subgraph/tests/addresslist-voting/addresslist-voting.test.ts

@@ -595,7 +595,6 @@ test('Run AddresslistVoting (handleMembersAdded) mappings with mock event', () =
     'id',
     memberEntityId
   );
-  const voter = AddresslistVotingVoter.load(memberEntityId);
   assert.fieldEquals(
     'AddresslistVotingVoter',
     memberEntityId,

packages/subgraph/tests/multisig/multisig.test.ts

@@ -156,7 +156,12 @@ test('Run Multisig (handleProposalCreated) mappings with mock event', () => {
     SNAPSHOT_BLOCK
   );
   assert.fieldEquals('MultisigProposal', proposalEntityId, 'minApprovals', ONE);
-  assert.fieldEquals('MultisigProposal', proposalEntityId, 'approvals', ONE);
+  assert.fieldEquals(
+    'MultisigProposal',
+    proposalEntityId,
+    'approvalCount',
+    ONE
+  );
   assert.fieldEquals('MultisigProposal', proposalEntityId, 'executed', 'false');
   assert.fieldEquals(
     'MultisigProposal',
@@ -221,32 +226,32 @@ test('Run Multisig (handleApproved) mappings with mock event', () => {
   const voterEntityId = generateVoterEntityId(memberEntityId, proposal.id);
   // check proposalVoter
   assert.fieldEquals(
-    'MultisigProposalApprover',
+    'MultisigProposalApproval',
     voterEntityId,
     'id',
     voterEntityId
   );
   assert.fieldEquals(
-    'MultisigProposalApprover',
+    'MultisigProposalApproval',
+    voterEntityId,
+    'createdAt',
+    event.block.timestamp.toString()
+  );
+  assert.fieldEquals(
+    'MultisigProposalApproval',
     voterEntityId,
     'approver',
     memberEntityId
   );
   assert.fieldEquals(
-    'MultisigProposalApprover',
+    'MultisigProposalApproval',
     voterEntityId,
     'proposal',
     proposal.id
   );
-  assert.fieldEquals(
-    'MultisigProposalApprover',
-    voterEntityId,
-    'createdAt',
-    event.block.timestamp.toString()
-  );
 
   // check proposal
-  assert.fieldEquals('MultisigProposal', proposal.id, 'approvals', ONE);
+  assert.fieldEquals('MultisigProposal', proposal.id, 'approvalCount', ONE);
   assert.fieldEquals(
     'MultisigProposal',
     proposal.id,
@@ -284,7 +289,7 @@ test('Run Multisig (handleApproved) mappings with mock event', () => {
   handleApproved(event2);
 
   // Check
-  assert.fieldEquals('MultisigProposal', proposal.id, 'approvals', TWO);
+  assert.fieldEquals('MultisigProposal', proposal.id, 'approvalCount', TWO);
   assert.fieldEquals(
     'MultisigProposal',
     proposal.id,
@@ -358,10 +363,10 @@ test('Run Multisig (handleMembersAdded) mappings with mock event', () => {
   handleMembersAdded(event);
 
   // checks
-  let memberId =
-    Address.fromString(CONTRACT_ADDRESS).toHexString() +
-    '_' +
-    userArray[0].toHexString();
+  let memberId = generateMemberEntityId(
+    Address.fromString(CONTRACT_ADDRESS),
+    userArray[0]
+  );
 
   assert.fieldEquals('MultisigApprover', memberId, 'id', memberId);
   assert.fieldEquals(
@@ -376,6 +381,7 @@ test('Run Multisig (handleMembersAdded) mappings with mock event', () => {
     'plugin',
     Address.fromString(CONTRACT_ADDRESS).toHexString()
   );
+  assert.fieldEquals('MultisigApprover', memberId, 'isActive', 'true');
 
   clearStore();
 });
@@ -386,25 +392,23 @@ test('Run Multisig (handleMembersRemoved) mappings with mock event', () => {
     Address.fromString(ADDRESS_ONE),
     Address.fromString(ADDRESS_TWO),
   ];
-
+  let pluginAddress = Address.fromString(CONTRACT_ADDRESS);
+  // create approvers
   for (let index = 0; index < memberAddresses.length; index++) {
-    const user = memberAddresses[index].toHexString();
-    const pluginId = Address.fromString(CONTRACT_ADDRESS).toHexString();
-    let memberId = pluginId + '_' + user;
-    let userEntity = new MultisigApprover(memberId);
-    userEntity.plugin = Address.fromString(CONTRACT_ADDRESS).toHexString();
-    userEntity.save();
+    let memberId = generateMemberEntityId(
+      pluginAddress,
+      memberAddresses[index]
+    );
+    let approverEntity = new MultisigApprover(memberId);
+    approverEntity.plugin = pluginAddress.toHexString();
+    approverEntity.address = memberAddresses[index];
+    approverEntity.isActive = true;
+    approverEntity.save();
   }
 
   // checks
-  let memberId1 =
-    Address.fromString(CONTRACT_ADDRESS).toHexString() +
-    '_' +
-    memberAddresses[0].toHexString();
-  let memberId2 =
-    Address.fromString(CONTRACT_ADDRESS).toHexString() +
-    '_' +
-    memberAddresses[1].toHexString();
+  let memberId1 = generateMemberEntityId(pluginAddress, memberAddresses[0]);
+  let memberId2 = generateMemberEntityId(pluginAddress, memberAddresses[1]);
 
   assert.fieldEquals('MultisigApprover', memberId1, 'id', memberId1);
   assert.fieldEquals('MultisigApprover', memberId2, 'id', memberId2);
@@ -420,8 +424,9 @@ test('Run Multisig (handleMembersRemoved) mappings with mock event', () => {
 
   // checks
   assert.fieldEquals('MultisigApprover', memberId1, 'id', memberId1);
-  assert.notInStore('MultisigApprover', memberId2);
-
+  assert.fieldEquals('MultisigApprover', memberId1, 'isActive', 'true');
+  assert.fieldEquals('MultisigApprover', memberId2, 'id', memberId2);
+  assert.fieldEquals('MultisigApprover', memberId2, 'isActive', 'false');
   clearStore();
 });
 

packages/subgraph/tests/token/governance-erc20.test.ts

@@ -17,7 +17,6 @@ import {getBalanceOf} from '../dao/utils';
 import {ExtendedTokenVotingMember} from '../helpers/extended-schema';
 import {
   createNewDelegateChangedEvent,
-  createNewERC20TransferEvent,
   createNewERC20TransferEventWithAddress,
   createTokenVotingMember,
   getDelegatee,
@@ -27,7 +26,7 @@ import {
   generateEntityIdFromAddress,
   generatePluginEntityId,
 } from '@aragon/osx-commons-subgraph';
-import {Address, BigInt, DataSourceContext, log} from '@graphprotocol/graph-ts';
+import {Address, BigInt, DataSourceContext} from '@graphprotocol/graph-ts';
 import {
   assert,
   afterEach,
@@ -610,8 +609,6 @@ describe('Governance ERC20', () => {
     test("It should initialize with the user's existing voting power and delegation, if she has any", () => {
       // constants
       const STARTING_BALANCE = '10';
-      const TRANSFER = '3';
-      const REMAINING = '7';
 
       // mocked calls
       getBalanceOf(
@@ -639,16 +636,6 @@ describe('Governance ERC20', () => {
         Address.fromString(fromAddressHexString)
       );
 
-      const memberEntityIdTo = generateMemberEntityId(
-        pluginAddressSecond,
-        Address.fromString(toAddressHexString)
-      );
-
-      const memberEntityIdToSecondPlugin = generateMemberEntityId(
-        pluginAddressSecond,
-        Address.fromString(toAddressHexString)
-      );
-
       // delegate to self
       const delegateChangedEvent = createNewDelegateChangedEvent(
         fromAddressHexString,