fix: mergeMultisigTransactions logic error

[AlgoDoggo]

This bug will manifest when the length of input array is > 2. Because all the mocha tests try to merge only two transactions it wasn't caught.

Essentially you have a for loop that iterates over all the transactions and at every iteration the newSubsigs array which should comprise all the pk and signatures from all the txs is overwritten with the the one from the reference case which is multisigTxnBlobs[0] + the ones from the current iteration of the loop. Thus when exiting the for loop the newSubsigs array is made of the signatures available in the reference tx + the last tx. All the other transactions signatures get overwritten.

It it is possible to fix this bug with a one-liner, right at the beginning of the map it would be enough to replace current with

const current = newSubsigs[index];

However this whole function in general is a little clunky so I couldn't help refactoring slightly the map statement to avoid nested if statement and such. The for loop too can be instantiated at index 1 since index 0 is the base case calculated above. If you don't like style changes the one-liner above is enough to do the job.

[AlgoDoggo]

@barnjamin could I have a bug-fix label so cicd passes plz?

[jasonpaulos]

Hi @AlgoDoggo, thanks for making this PR. I only have one request: can you add a new test that merges > 2 multisigs, since you said that was missing from our test suite?

I need to take a closer look at the code changes to be absolutely sure they work as intended, and a new test would help with that effort (in addition to being a regression test).

[scholtz]

Please fix mergeMultisigTransactions .. it took us also quite some time that documented functionality does not work.

[AlgoDoggo]

Hey, I've refactored a test to merge three tx instead of two. You can run it solo against the old and new logic to see the diff.

[jasonpaulos]

I believe this code change is correct, and I have independently verified the encoded test transactions.

I only have two small comments about the maintainability of the code.

[AlgoDoggo]

At some point we'll need to run typescript in strict mode, or at least with strictNullChecks enabled. A lot of values that can be undefined are just assumed to exist now by the sdk. It's bound to cause runtime errors eventually.

[AlgoDoggo]

@jasonpaulos Looks like cicd is failing at the npm ci step, chromedriver won't install. I'd try rerunning the workflows but I do not have permissions for this.

[AlgoDoggo]

ah nvm, the dev branch had moved.