Added check in validate cosignatories for

modify multisig account (removal)

Author: rg911

src/service/AggregatedTransactionService.ts

@@ -20,7 +20,11 @@ import { TransactionMapping } from '../core/utils/TransactionMapping';
 import { AccountHttp } from '../infrastructure/AccountHttp';
 import { MultisigAccountGraphInfo } from '../model/account/MultisigAccountGraphInfo';
 import { AggregateTransaction } from '../model/transaction/AggregateTransaction';
+import { InnerTransaction } from '../model/transaction/InnerTransaction';
+import { ModifyMultisigAccountTransaction } from '../model/transaction/ModifyMultisigAccountTransaction';
+import { MultisigCosignatoryModificationType } from '../model/transaction/MultisigCosignatoryModificationType';
 import { SignedTransaction } from '../model/transaction/SignedTransaction';
+import { TransactionType } from '../model/transaction/TransactionType';
 
 /**
  * Aggregated Transaction service
@@ -58,7 +62,7 @@ export class AggregatedTransactionService {
                     mergeMap((_) => _.minApproval !== 0 && _.minRemoval !== 0 ?
                         this.accountHttp.getMultisigAccountGraphInfo(_.account.address)
                         .pipe(
-                            map((graphInfo) => this.validateCosignatories(graphInfo, signers)),
+                            map((graphInfo) => this.validateCosignatories(graphInfo, signers, innerTransaction)),
                         ) : observableOf(true),
                         ),
                     ),
@@ -70,21 +74,37 @@ export class AggregatedTransactionService {
      * Validate cosignatories against multisig Account(s)
      * @param graphInfo - multisig account graph info
      * @param cosignatories - array of cosignatories extracted from aggregated transaction
+     * @param innerTransaction - the inner transaction of the aggregated transaction
      * @returns {boolean}
      */
-    private validateCosignatories(graphInfo: MultisigAccountGraphInfo, cosignatories: string[]): boolean {
+    private validateCosignatories(graphInfo: MultisigAccountGraphInfo,
+                                  cosignatories: string[],
+                                  innerTransaction: InnerTransaction): boolean {
         /**
          *  Validate cosignatories from bottom level to top
          */
         const sortedKeys = Array.from(graphInfo.multisigAccounts.keys()).sort((a, b) => b - a);
         const cosignatoriesReceived = cosignatories;
         let validationResult = true;
 
+        let isMultisigRemoval = false;
+
+        /**
+         * Check inner transaction. If remove cosigner from multisig account,
+         * use minRemoval instead of minApproval for cosignatories validation.
+         */
+        if (innerTransaction.type === TransactionType.MODIFY_MULTISIG_ACCOUNT) {
+            if ((innerTransaction as ModifyMultisigAccountTransaction).modifications
+                    .find((modification) => modification.type === MultisigCosignatoryModificationType.Remove) !== undefined) {
+                        isMultisigRemoval = true;
+            }
+        }
+
         sortedKeys.forEach((key) => {
             const multisigInfo = graphInfo.multisigAccounts.get(key);
             if (multisigInfo && validationResult) {
                 multisigInfo.forEach((multisig) => {
-                    if (multisig.minApproval >= 1) {
+                    if (multisig.minApproval >= 1 && multisig.minRemoval) { // To make sure it is multisig account
                         const matchedCosignatories = this.compareArrays(cosignatoriesReceived,
                                         multisig.cosignatories.map((cosig) => cosig.publicKey));
 
@@ -93,7 +113,8 @@ export class AggregatedTransactionService {
                          * into the received signatories array for next level validation.
                          * Otherwise return validation failed.
                          */
-                        if (matchedCosignatories.length >= multisig.minApproval) {
+                        if ((matchedCosignatories.length >= multisig.minApproval && !isMultisigRemoval) ||
+                            (matchedCosignatories.length >= multisig.minRemoval && isMultisigRemoval)) {
                             if (cosignatoriesReceived.indexOf(multisig.account.publicKey) === -1) {
                                 cosignatoriesReceived.push(multisig.account.publicKey);
                               }

test/service/AggregatedTransactionService.spec.ts

@@ -26,6 +26,9 @@ import { MultisigAccountInfo } from '../../src/model/account/MultisigAccountInfo
 import {NetworkType} from '../../src/model/blockchain/NetworkType';
 import { AggregateTransaction } from '../../src/model/transaction/AggregateTransaction';
 import { Deadline } from '../../src/model/transaction/Deadline';
+import { ModifyMultisigAccountTransaction } from '../../src/model/transaction/ModifyMultisigAccountTransaction';
+import { MultisigCosignatoryModification } from '../../src/model/transaction/MultisigCosignatoryModification';
+import { MultisigCosignatoryModificationType } from '../../src/model/transaction/MultisigCosignatoryModificationType';
 import { PlainMessage } from '../../src/model/transaction/PlainMessage';
 import { TransferTransaction } from '../../src/model/transaction/TransferTransaction';
 import { AggregatedTransactionService } from '../../src/service/AggregatedTransactionService';
@@ -177,6 +180,30 @@ describe('AggregatedTransactionService', () => {
         });
     });
 
+    it('should use minRemoval for multisig account validation if inner transaction is modify multisig remove', () => {
+        const modifyMultisigTransaction = ModifyMultisigAccountTransaction.create(
+            Deadline.create(1, ChronoUnit.HOURS),
+            1,
+            1,
+            [new MultisigCosignatoryModification(
+                MultisigCosignatoryModificationType.Remove,
+                account1.publicAccount,
+            )],
+            NetworkType.MIJIN_TEST,
+        );
+
+        const aggregateTransaction = AggregateTransaction.createComplete(
+            Deadline.create(),
+            [modifyMultisigTransaction.toAggregate(multisig2.publicAccount)],
+            NetworkType.MIJIN_TEST,
+            []);
+
+        const signedTransaction = aggregateTransaction.signWith(account2);
+        aggregatedTransactionService.isComplete(signedTransaction).toPromise().then((isComplete) => {
+            expect(isComplete).to.be.true;
+        });
+    });
+
     it('should return correct isComplete status for aggregated complete transaction - none multisig', () => {
         const transferTransaction = TransferTransaction.create(
             Deadline.create(1, ChronoUnit.HOURS),