Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

atc.gatherSignatures() failing with Multisig #821

Closed
acfunk opened this issue Aug 28, 2023 · 7 comments
Closed

atc.gatherSignatures() failing with Multisig #821

acfunk opened this issue Aug 28, 2023 · 7 comments
Labels
new-bug Bug report that needs triage

Comments

@acfunk
Copy link
Contributor

acfunk commented Aug 28, 2023

atc.gatherSignatures() failing with Multisig

When gathering signatures for an AtomicTransactionComposer group with a multisig transaction, the following error is thrown:

Not enough multisig transactions to merge. Need at least two

Your environment

algosdk 2.5.0

Steps to reproduce

  1. Create an ATC with one or more TransactionWithSigners that have a MultiSig signer
  2. const signedTxns = await atc.gatherSignatures()

Expected behaviour

signedTxns contains signed transactions

Actual behaviour

error is thrown Not enough multisig transactions to merge. Need at least two

@acfunk acfunk added the new-bug Bug report that needs triage label Aug 28, 2023
@winder
Copy link
Contributor

winder commented Aug 29, 2023

Are you using a multisig that only requires one signature?

@acfunk
Copy link
Contributor Author

acfunk commented Aug 29, 2023

@winder Yes, it's a 1 of 2 multi-sig. I started looking into the problem and realized that was a factor. Sorry it wasn't in the repro steps.

I figure something like this in place of this line would fix it...

signed.push(mergeMultisigTransactions(partialSigs));

  if(partialSigs.length > 1) {
    signed.push(mergeMultisigTransactions(partialSigs));
  } else {
    signed.push(partialSigs[0]);
  }

@winder
Copy link
Contributor

winder commented Aug 29, 2023

@acfunk thanks, that sounds right to me as well. Would you like to make a PR?

@acfunk
Copy link
Contributor Author

acfunk commented Aug 29, 2023

@winder I was going to, but I couldn't figure out how to test it locally. Editing the file in node_modules had no effect - too much optimization by Vite it seems. And I'd feel bad submitting a PR that I hadn't tested... unless you told me to.

@winder
Copy link
Contributor

winder commented Aug 29, 2023

@algochoi could you provide some testing guidance here?

@algochoi
Copy link
Contributor

We could have a mocha unit test here: https://github.com/algorand/js-algorand-sdk/blob/develop/tests/10.ABI.ts

something like:

it('should accept at least one signature in the multisig', () => {
// ...
});

@acfunk
Copy link
Contributor Author

acfunk commented Aug 30, 2023

I submitted a PR #822

@winder winder closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-bug Bug report that needs triage
Projects
None yet
Development

No branches or pull requests

3 participants