feat: convert to or modify multisig account added #307

yilmazbahadir · 2021-10-30T20:39:02Z

symbol-bootstrap modifyMultisig command is added. It's useful to convert main account into a multisig account or to modify the multisig account structure. Takes the following parameters, the user will be prompted for the unset parameters.

-A, --addressAdditions=addressAdditions              Cosignatory accounts addresses to be added (separated by a
                                                       comma).

  -D, --addressDeletions=addressDeletions              Cosignatory accounts addresses to be removed (separated by a
                                                       comma).

  -a, --minApprovalDelta=minApprovalDelta              Number of signatures needed to approve a transaction. If the
                                                       account is already multisig, enter the number of cosignatories to
                                                       add or remove.

  -r, --minRemovalDelta=minRemovalDelta                Number of signatures needed to remove a cosignatory. If the
                                                       account is already multisig, enter the number of cosignatories to
                                                       add or remove.

rg911 · 2021-10-30T22:03:32Z

src/service/AnnounceService.ts

-                            ready,
-                            nodeAccount.name,
-                        );
+                        if (transactions[0] instanceof MultisigAccountModificationTransaction) {


would it be better to use the transactionType instead?

why do you think so?

I mean you can use instanceof, but the SDK has already got a transactionType enum. could that be neat using the type?

rg911 · 2021-10-30T22:12:18Z

src/service/AnnounceService.ts

+                                ready,
+                                nodeAccount.name,
+                            );
+                        }
                    } else {
                        await this.announceAggregateComplete(


does the modifyMultisig tx needs to be announced as a bonded transaction? Using transactions.length might not always be safe. What if modifyMultisig is part of the transactions array? what about using array.filter?

rg911 · 2021-10-30T22:15:05Z

src/service/ModifyMultisigService.ts

+        target: BootstrapUtils.defaultTargetFolder,
+        useKnownRestGateways: false,
+        ready: false,
+        url: 'http://localhost:3000',


default to hardcoded http?

yes and default to localhost :)
Copied from: https://github.com/symbol/symbol-bootstrap/blob/56239efa3100116ea294c570ddab317160e0e00b/src/service/LinkService.ts#L77
I think it's fine for this PR, the node list loading logic will be changed after @fboucquez 's (soon to be raised) PR.

Yes, it's fine, default url is when you are running the link/modify command on the same box that you have launched the node. Most people would either provide an --url or use --useKnownRestGateways

fboucquez

Please also run npm run style:fix to update the index.ts and docs

I have done the following testing:

Multisig M, cosigners A,B,C , ServiceOperatorAccount S. It looks really nice:

Convert to multisig M using M private key. It creates a bonded where All A,B,C need to cosign.
Modify mutlisg M using A cosigner private key. It creates a bonded where some of B,C to cosign
Convert to multisig M using S private key. It creates a bonded where all M, A, B, C need to cosign (wallet warning shown as expected)
Modify multisig M using S private key. It creates a bonded where some A, B, C need to cosign (wallet warning shown as expected)

Could the command ask (optionally) for cosigners private keys. Use case, I'm a regular user that wants to convert my main account to multisig for security reasons. I could provide my cosigners private keys on the spot without bonded, using full aggregate complete. This could be an improvement though, the service providers should never know any cosigner private key.

fboucquez · 2021-10-31T14:40:30Z

src/commands/modifyMultisig.ts

+            char: 'a',
+        }),
+        addressAdditions: flags.string({
+            description: 'Cosignatory accounts addresses to be added (separated by a comma).',


have you tried multiple: true, cli handles lists of strings. Does it work nicely?

Yes, tried multiple addresses separated by comma, works fine 👍

I mean, oclif multiple: true field in the flags.string. It uses cli style multiple values

ignore, --addressAdditions A,B,C is easier than --addressAdditions A --addressAdditions B --addressAdditions C

does --addressAdditions (empty) work? empty list no prompt.

I think so

ignore, --addressAdditions A,B,C is easier than --addressAdditions A --addressAdditions B --addressAdditions C

No it doesn't, it gives => No Error: Flag --addressAdditions expects a value

does --addressAdditions (empty) work? empty list no prompt.

If we need to support no-prompt command as well, I think we have 2 options here:

Remove all the prompts, use only the flag values

Add the following flags --no-addressAdditions, --no-addressDeletions
and the command will look like
symbol-bootstrap modifyMultisig --no-addressAdditions --no-addressDeletions --minApprovalDelta=1 --minRemovalDelta=0
I'd probably vote for 2.

Updated the code as it will not prompt for the following.
--addressAdditions='' --addressDeletions=''

fboucquez · 2021-10-31T14:44:34Z

src/service/ModifyMultisigService.ts

+        target: BootstrapUtils.defaultTargetFolder,
+        useKnownRestGateways: false,
+        ready: false,
+        url: 'http://localhost:3000',


Yes, it's fine, default url is when you are running the link/modify command on the same box that you have launched the node. Most people would either provide an --url or use --useKnownRestGateways

yilmazbahadir · 2021-10-31T15:39:22Z

Please also run npm run style:fix to update the index.ts and docs

I have done the following testing:

Multisig M, cosigners A,B,C , ServiceOperatorAccount S. It looks really nice:

Convert to multisig M using M private key. It creates a bonded where All A,B,C need to cosign.

Modify mutlisg M using A cosigner private key. It creates a bonded where some of B,C to cosign

Convert to multisig M using S private key. It creates a bonded where all M, A, B, C need to cosign (wallet warning shown as expected)

Modify multisig M using S private key. It creates a bonded where some A, B, C need to cosign (wallet warning shown as expected)

Could the command ask (optionally) for cosigners private keys. Use case, I'm a regular user that wants to convert my main account to multisig for security reasons. I could provide my cosigners private keys on the spot without bonded, using full aggregate complete. This could be an improvement though, the service providers should never know any cosigner private key.

Great! Will run the style:fix script. Let's implement this improvement with a different PR, created https://github.com/symbol/symbol-bootstrap/issues/308.
Currently writing some unit tests and also adding some parameter sanity checks, will be updating the PR shortly.

sonarqubecloud · 2021-10-31T21:03:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

feat: convert to or modify multisig account added

009d108

yilmazbahadir force-pushed the feature/multisig-account-modification branch from 2972581 to 009d108 Compare October 30, 2021 20:50

yilmazbahadir requested a review from fboucquez October 30, 2021 20:51

rg911 reviewed Oct 30, 2021

View reviewed changes

Baha added 2 commits October 30, 2021 23:23

fix: sonar warnings

fe88acd

transaction instanceof check replaced with transactionType

043e741

fboucquez requested changes Oct 31, 2021

View reviewed changes

Baha and others added 3 commits October 31, 2021 20:12

parameter verification and unit tests added

804f447

Merge branch 'dev' into feature/multisig-account-modification

67a1cd1

updates for no-prompt command

da86d0b

fboucquez merged commit 67287bc into dev Oct 31, 2021

fboucquez deleted the feature/multisig-account-modification branch October 31, 2021 21:24

fboucquez mentioned this pull request Nov 2, 2021

Add command to convert main account to multisig #305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert to or modify multisig account added #307

feat: convert to or modify multisig account added #307

yilmazbahadir commented Oct 30, 2021 •

edited

Loading

rg911 Oct 30, 2021

yilmazbahadir Oct 30, 2021

rg911 Oct 30, 2021

rg911 Oct 30, 2021

rg911 Oct 30, 2021

yilmazbahadir Oct 30, 2021

fboucquez Oct 31, 2021

fboucquez left a comment •

edited

Loading

fboucquez Oct 31, 2021

yilmazbahadir Oct 31, 2021

fboucquez Oct 31, 2021 •

edited

Loading

fboucquez Oct 31, 2021 •

edited

Loading

yilmazbahadir Oct 31, 2021

yilmazbahadir Oct 31, 2021

fboucquez Oct 31, 2021

yilmazbahadir commented Oct 31, 2021

sonarqubecloud bot commented Oct 31, 2021

feat: convert to or modify multisig account added #307

feat: convert to or modify multisig account added #307

Conversation

yilmazbahadir commented Oct 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboucquez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboucquez Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

fboucquez Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yilmazbahadir commented Oct 31, 2021

sonarqubecloud bot commented Oct 31, 2021

yilmazbahadir commented Oct 30, 2021 •

edited

Loading

fboucquez left a comment •

edited

Loading

fboucquez Oct 31, 2021 •

edited

Loading

fboucquez Oct 31, 2021 •

edited

Loading