Merge pull request #516 from algorandfoundation/feat/gather-signatures

feat: return bytes from gatherSignatures to prevent double encoding

Author: neilcampbell

docs/code/classes/types_composer.TransactionComposer.md

@@ -24,7 +24,7 @@ Any `ABIMethod` objects associated with any of the transactions in a map keyed b
 
 #### Defined in
 
-[src/types/composer.ts:220](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L220)
+[src/types/composer.ts:218](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L218)
 
 ___
 
@@ -36,7 +36,7 @@ Any `TransactionSigner` objects associated with any of the transactions in a map
 
 #### Defined in
 
-[src/types/composer.ts:222](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L222)
+[src/types/composer.ts:220](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L220)
 
 ___
 
@@ -48,4 +48,4 @@ The built transactions
 
 #### Defined in
 
-[src/types/composer.ts:218](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L218)
+[src/types/composer.ts:216](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L216)

docs/code/interfaces/types_composer.BuiltTransactions.md

@@ -344,7 +344,7 @@ and return the input error if it cannot or should not transform it.
 
 #### Defined in
 
-[src/types/composer.ts:157](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L157)
+[src/types/composer.ts:155](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L155)
 
 ___
 
@@ -423,7 +423,7 @@ See algod API docs for more information: https://dev.algorand.co/reference/rest-
 
 #### Defined in
 
-[src/types/composer.ts:128](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L128)
+[src/types/composer.ts:126](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L126)
 
 ___
 
@@ -435,7 +435,7 @@ All options to control a simulate request
 
 #### Defined in
 
-[src/types/composer.ts:134](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L134)
+[src/types/composer.ts:132](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L132)
 
 ___
 
@@ -447,7 +447,7 @@ Options to control a simulate request, that does not require transaction signing
 
 #### Defined in
 
-[src/types/composer.ts:116](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L116)
+[src/types/composer.ts:114](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L114)
 
 ___
 
@@ -464,7 +464,7 @@ ___
 
 #### Defined in
 
-[src/types/composer.ts:171](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L171)
+[src/types/composer.ts:169](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L169)
 
 ___
 
@@ -488,4 +488,4 @@ Parameters to create an `TransactionComposer`.
 
 #### Defined in
 
-[src/types/composer.ts:191](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L191)
+[src/types/composer.ts:189](https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/types/composer.ts#L189)

docs/code/modules/types_composer.md

@@ -1,7 +1,13 @@
 import { Address, Addressable, concatArrays, decodeMsgpack, hash } from '@algorandfoundation/algokit-common'
 import { MultisigAccount } from './multisig'
 import { AddressWithDelegatedLsigSigner, TransactionSigner } from './signer'
-import { LogicSigSignature, MultisigSignature, SignedTransaction, encodeSignedTransaction } from './transactions/signed-transaction'
+import {
+  LogicSigSignature,
+  MultisigSignature,
+  SignedTransaction,
+  encodeSignedTransaction,
+  validateSignedTransaction,
+} from './transactions/signed-transaction'
 import { logicSigSignatureCodec } from './transactions/signed-transaction-meta'
 import { Transaction } from './transactions/transaction'
 
@@ -137,6 +143,7 @@ export class LogicSigAccount extends LogicSig {
           stxn.authAddress = this.addr
         }
 
+        validateSignedTransaction(stxn)
         signedTxns.push(encodeSignedTransaction(stxn))
       }
 

packages/transact/src/logicsig.ts

@@ -8,16 +8,17 @@ import {
   PUBLIC_KEY_BYTE_LENGTH,
   SIGNATURE_BYTE_LENGTH,
 } from '@algorandfoundation/algokit-common'
+import { DelegatedLsigSigner } from './logicsig'
 import { AddressWithDelegatedLsigSigner, AddressWithTransactionSigner, TransactionSigner } from './signer'
 import {
   decodeSignedTransaction,
   encodeSignedTransaction,
   MultisigSignature,
   MultisigSubsignature,
   SignedTransaction,
+  validateSignedTransaction,
 } from './transactions/signed-transaction'
 import { Transaction } from './transactions/transaction'
-import { DelegatedLsigSigner } from './logicsig'
 
 const toPublicKeys = (addrs: Array<string | Address>): Uint8Array[] => addrs.map((addr) => getAddress(addr).publicKey)
 
@@ -451,7 +452,10 @@ export class MultisigAccount implements AddressWithTransactionSigner, AddressWit
         signedMsigTxns.push(signedMsigTxn)
       }
 
-      return signedMsigTxns.map(encodeSignedTransaction)
+      return signedMsigTxns.map((stxn) => {
+        validateSignedTransaction(stxn)
+        return encodeSignedTransaction(stxn)
+      })
     }
 
     this._lsigSigner = async (lsig, _) => {

packages/transact/src/multisig.ts

@@ -1,7 +1,7 @@
 import { Address, Addressable, concatArrays, Expand, ReadableAddress } from '@algorandfoundation/algokit-common'
-import { encodeTransaction, Transaction } from './transactions/transaction'
 import { DelegatedLsigSigner, ProgramDataSigner } from './logicsig'
-import { encodeSignedTransaction, SignedTransaction } from './transactions/signed-transaction'
+import { encodeSignedTransaction, SignedTransaction, validateSignedTransaction } from './transactions/signed-transaction'
+import { encodeTransaction, Transaction } from './transactions/transaction'
 
 /** Function for signing a group of transactions */
 export type TransactionSigner = (txnGroup: Transaction[], indexesToSign: number[]) => Promise<Uint8Array[]>
@@ -70,7 +70,10 @@ export function generateAddressWithSigners(args: {
       stxns.push(stxn)
     }
 
-    return stxns.map(encodeSignedTransaction)
+    return stxns.map((stxn) => {
+      validateSignedTransaction(stxn)
+      return encodeSignedTransaction(stxn)
+    })
   }
 
   const lsigSigner: DelegatedLsigSigner = async (lsig, msig) => {
@@ -105,6 +108,7 @@ export function makeEmptyTransactionSigner(): TransactionSigner {
         txn: txnGroup[index],
         sig: new Uint8Array(64).fill(0),
       }
+      validateSignedTransaction(stxn)
       unsigned.push(encodeSignedTransaction(stxn))
     }
 

packages/transact/src/signer.ts

@@ -485,8 +485,7 @@ describe('TransactionComposer', () => {
       const signedTxns = await composer.gatherSignatures()
 
       expect(signedTxns).toHaveLength(1)
-      expect(signedTxns[0]).toBeDefined()
-      expect(signedTxns[0].sig).toBeDefined()
+      expect(signedTxns[0].length).toBeGreaterThan(0)
     })
 
     test('should successfully sign multiple transactions with the same signer', async () => {
@@ -508,8 +507,8 @@ describe('TransactionComposer', () => {
       const signedTxns = await composer.gatherSignatures()
 
       expect(signedTxns).toHaveLength(2)
-      expect(signedTxns[0].sig).toBeDefined()
-      expect(signedTxns[1].sig).toBeDefined()
+      expect(signedTxns[0].length).toBeGreaterThan(0)
+      expect(signedTxns[1].length).toBeGreaterThan(0)
     })
 
     test('should successfully sign transactions with multiple different signers', async () => {
@@ -532,8 +531,8 @@ describe('TransactionComposer', () => {
       const signedTxns = await composer.gatherSignatures()
 
       expect(signedTxns).toHaveLength(2)
-      expect(signedTxns[0].sig).toBeDefined()
-      expect(signedTxns[1].sig).toBeDefined()
+      expect(signedTxns[0].length).toBeGreaterThan(0)
+      expect(signedTxns[1].length).toBeGreaterThan(0)
     })
 
     test('should throw error when no transactions to sign', async () => {
@@ -613,25 +612,5 @@ describe('TransactionComposer', () => {
 
       await expect(composer.gatherSignatures()).rejects.toThrow('Transactions at indexes [0] were not signed')
     })
-
-    test('should throw error when signer returns invalid signed transaction data', async () => {
-      const { algorand, context } = fixture
-      const sender = context.testAccount
-
-      // Create a faulty signer that returns invalid data
-      const faultySigner: TransactionSigner = async (_group, indexes) => {
-        return indexes.map(() => new Uint8Array([1, 2, 3]))
-      }
-
-      const composer = algorand.newGroup()
-      composer.addPayment({
-        sender,
-        receiver: sender,
-        amount: AlgoAmount.MicroAlgo(1000),
-        signer: faultySigner,
-      })
-
-      await expect(composer.gatherSignatures()).rejects.toThrow('Invalid signed transaction at index 0')
-    })
   })
 })

src/types/composer.spec.ts

@@ -16,13 +16,11 @@ import {
   TransactionType,
   assignFee,
   calculateFee,
-  decodeSignedTransaction,
+  decodeSignedTransactions,
   decodeTransaction,
-  encodeSignedTransactions,
   encodeTransactionRaw,
   groupTransactions,
   makeEmptyTransactionSigner,
-  validateSignedTransaction,
   validateTransaction,
 } from '@algorandfoundation/algokit-transact'
 import { Buffer } from 'buffer'
@@ -250,7 +248,7 @@ export class TransactionComposer {
 
   private transactionsWithSigners?: TransactionWithSigner[]
 
-  private signedTransactions?: SignedTransaction[]
+  private signedTransactions?: Uint8Array[]
 
   // Cache the raw transactions before resource population for error handling
   private rawBuildTransactions?: Transaction[]
@@ -1858,19 +1856,18 @@ export class TransactionComposer {
         })
       }
 
-      const group = this.signedTransactions[0].txn.group
+      const group = this.transactionsWithSigners[0].txn.group
 
       let waitRounds = params?.maxRoundsToWaitForConfirmation
 
       if (waitRounds === undefined) {
         const suggestedParams = await this.getSuggestedParams()
         const firstRound = suggestedParams.firstValid
-        const lastRound = this.signedTransactions.reduce((max, txn) => (txn.txn.lastValid > max ? txn.txn.lastValid : max), 0n)
+        const lastRound = this.transactionsWithSigners.reduce((max, txn) => (txn.txn.lastValid > max ? txn.txn.lastValid : max), 0n)
         waitRounds = Number(lastRound - firstRound) + 1
       }
 
-      const encodedTxns = encodeSignedTransactions(this.signedTransactions)
-      await this.algod.sendRawTransaction(encodedTxns)
+      await this.algod.sendRawTransaction(this.signedTransactions)
 
       if (transactionsToSend.length > 1 && group) {
         Config.getLogger(params?.suppressLog).verbose(
@@ -1927,7 +1924,9 @@ export class TransactionComposer {
           txn,
           signer: makeEmptyTransactionSigner(),
         }))
-        const signedTransactions = await this.signTransactions(transactionsWithEmptySigners)
+        const encodedSignedTransactions = await this.signTransactions(transactionsWithEmptySigners)
+        const signedTransactions = decodeSignedTransactions(encodedSignedTransactions)
+
         const simulateResponse = await this.algod.simulateTransactions({
           txnGroups: [{ txns: signedTransactions }],
           allowEmptySignatures: true,
@@ -2030,7 +2029,8 @@ export class TransactionComposer {
     }
 
     const transactions = transactionsWithSigner.map((e) => e.txn)
-    const signedTransactions = await this.signTransactions(transactionsWithSigner)
+    const encodedSignedTransactions = await this.signTransactions(transactionsWithSigner)
+    const signedTransactions = decodeSignedTransactions(encodedSignedTransactions)
 
     const simulateRequest = {
       txnGroups: [
@@ -2097,7 +2097,7 @@ export class TransactionComposer {
     return encoder.encode(arc2Payload)
   }
 
-  public async gatherSignatures(): Promise<SignedTransaction[]> {
+  public async gatherSignatures(): Promise<Uint8Array[]> {
     if (this.signedTransactions) {
       return this.signedTransactions
     }
@@ -2112,7 +2112,7 @@ export class TransactionComposer {
     return this.signedTransactions
   }
 
-  private async signTransactions(transactionsWithSigners: TransactionWithSigner[]): Promise<SignedTransaction[]> {
+  private async signTransactions(transactionsWithSigners: TransactionWithSigner[]): Promise<Uint8Array[]> {
     if (transactionsWithSigners.length === 0) {
       throw new Error('No transactions available to sign')
     }
@@ -2132,40 +2132,24 @@ export class TransactionComposer {
     const signedGroups = await Promise.all(signerEntries.map(([signer, indexes]) => signer(transactions, indexes)))
 
     // Reconstruct signed transactions in original order
-    const rawSignedTransactions: (Uint8Array | null)[] = new Array(transactionsWithSigners.length).fill(null)
+    const encodedSignedTransactions: (Uint8Array | null)[] = new Array(transactionsWithSigners.length).fill(null)
     signerEntries.forEach(([, indexes], signerIndex) => {
       const stxs = signedGroups[signerIndex]
       indexes.forEach((txIndex, stxIndex) => {
-        rawSignedTransactions[txIndex] = stxs[stxIndex] ?? null
+        encodedSignedTransactions[txIndex] = stxs[stxIndex] ?? null
       })
     })
 
     // Verify all transactions were signed
-    const unsignedIndexes = rawSignedTransactions
+    const unsignedIndexes = encodedSignedTransactions
       .map((stxn, index) => (stxn == null ? index : null))
       .filter((index): index is number => index !== null)
 
     if (unsignedIndexes.length > 0) {
       throw new Error(`Transactions at indexes [${unsignedIndexes.join(', ')}] were not signed`)
     }
 
-    // Decode and validate all signed transactions
-    const signedTransactions = rawSignedTransactions.map((stxn, index) => {
-      if (stxn == null) {
-        // This shouldn't happen due to the check above, but ensures type safety
-        throw new Error(`Transaction at index ${index} was not signed`)
-      }
-
-      try {
-        const signedTransaction = decodeSignedTransaction(stxn)
-        validateSignedTransaction(signedTransaction)
-        return signedTransaction
-      } catch (err) {
-        throw new Error(`Invalid signed transaction at index ${index}. ${err}`)
-      }
-    })
-
-    return signedTransactions
+    return encodedSignedTransactions as Uint8Array[] // The guard above ensures no nulls
   }
 
   private parseAbiReturnValues(confirmations: PendingTransactionResponse[]): ABIReturn[] {