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

feat: return bytes from gatherSignatures to prevent double encoding

Author: neilcampbell

docs/markdown/autoapi/algokit_utils/transactions/transaction_composer/index.md

@@ -174,7 +174,7 @@ Build queued transactions without resource population or grouping.
 Returns raw transactions, method call metadata, and any explicit signers. This does not
 populate unnamed resources or adjust fees, and it leaves grouping unchanged.
 
-#### gather_signatures() → list[algokit_transact.models.signed_transaction.SignedTransaction]
+#### gather_signatures() → list[bytes]
 
 #### send(params: [algokit_utils.models.transaction.SendParams](../../models/transaction/index.md#algokit_utils.models.transaction.SendParams) | None = None) → [SendTransactionComposerResults](#algokit_utils.transactions.transaction_composer.SendTransactionComposerResults)
 

src/algokit_transact/logicsig.py

@@ -8,6 +8,7 @@
 from algokit_transact.codec.signed import encode_signed_transaction
 from algokit_transact.models.signed_transaction import SignedTransaction
 from algokit_transact.models.transaction import Transaction
+from algokit_transact.ops.validate import validate_signed_transaction
 from algokit_transact.signing.logic_signature import LogicSigSignature
 from algokit_transact.signing.types import MultisigSignature
 from algokit_transact.signing.validation import sanity_check_program
@@ -155,6 +156,7 @@ def signer(txn_group: Sequence[Transaction], indexes_to_sign: Sequence[int]) ->
                     lsig=logic_sig,
                     auth_address=auth_addr,
                 )
+                validate_signed_transaction(signed)
                 blobs.append(encode_signed_transaction(signed))
             return blobs
 

src/algokit_transact/models/state_proof.py

@@ -83,7 +83,7 @@ class StateProofMessage:
 
 @dataclass(slots=True, frozen=True)
 class StateProofTransactionFields:
-    state_proof_type: int | None = field(default=None, metadata=wire("sptype"))
+    state_proof_type: int = field(default=0, metadata=wire("sptype"))
     # Flatten state proof and message at the transaction level (aliases under root: sp and spmsg)
     state_proof: StateProof | None = field(default=None, metadata=nested("sp", StateProof))
     message: StateProofMessage | None = field(default=None, metadata=nested("spmsg", StateProofMessage))

src/algokit_transact/multisig.py

@@ -8,6 +8,7 @@
 from algokit_transact.codec.transaction import encode_transaction
 from algokit_transact.models.signed_transaction import SignedTransaction
 from algokit_transact.models.transaction import Transaction as AlgokitTransaction
+from algokit_transact.ops.validate import validate_signed_transaction
 from algokit_transact.signer import AddressWithSigners
 from algokit_transact.signing.multisig import (
     address_from_multisig_signature,
@@ -87,6 +88,7 @@ def signer(txn_group: Sequence[AlgokitTransaction], indexes_to_sign: Sequence[in
                     lsig=None,
                     auth_address=msig_address if txn.sender != msig_address else None,
                 )
+                validate_signed_transaction(signed)
                 blobs.append(encode_signed_transaction(signed))
             return blobs
 

src/algokit_transact/ops/validate.py

@@ -20,14 +20,14 @@
     PROGRAM_PAGE_SIZE,
     SIGNATURE_BYTE_LENGTH,
 )
-from algokit_transact import SignedTransaction
 from algokit_transact.exceptions import TransactionValidationError
 from algokit_transact.models.app_call import AppCallTransactionFields
 from algokit_transact.models.asset_config import AssetConfigTransactionFields
 from algokit_transact.models.asset_freeze import AssetFreezeTransactionFields
 from algokit_transact.models.asset_transfer import AssetTransferTransactionFields
 from algokit_transact.models.common import OnApplicationComplete
 from algokit_transact.models.key_registration import KeyRegistrationTransactionFields
+from algokit_transact.models.signed_transaction import SignedTransaction
 from algokit_transact.models.transaction import Transaction
 
 
@@ -60,9 +60,8 @@ def _issue(
 def validate_signed_transaction(stx: SignedTransaction) -> None:
     validate_transaction(stx.txn)
 
-    signatures = {stx.sig, stx.msig, stx.lsig} - {None}
-    if not signatures:
-        raise ValueError("At least one signature type must be set")
+    signatures = [s for s in (stx.sig, stx.msig, stx.lsig) if s is not None]
+
     if len(signatures) > 1:
         raise ValueError("Only one signature type can be set")
 

src/algokit_transact/signer.py

@@ -13,6 +13,7 @@
 from algokit_transact.codec.transaction import encode_transaction
 from algokit_transact.models.signed_transaction import SignedTransaction
 from algokit_transact.models.transaction import Transaction
+from algokit_transact.ops.validate import validate_signed_transaction
 
 if TYPE_CHECKING:
     from algokit_transact.logicsig import (
@@ -115,6 +116,7 @@ def transaction_signer(txn_group: Sequence[Transaction], indexes_to_sign: Sequen
                 sig=signature,
                 auth_address=auth_addr if txn.sender != auth_addr else None,
             )
+            validate_signed_transaction(stxn)
             result.append(encode_signed_transaction(stxn))
         return result
 
@@ -153,6 +155,7 @@ def empty_signer(txn_group: Sequence[Transaction], indexes_to_sign: Sequence[int
                 txn=txn_group[index],
                 sig=EMPTY_SIGNATURE,
             )
+            validate_signed_transaction(stxn)
             result.append(encode_signed_transaction(stxn))
         return result
 

src/algokit_utils/transactions/transaction_composer.py

@@ -11,13 +11,13 @@
 from algokit_algod_client.exceptions import UnexpectedStatusError
 from algokit_algod_client.models import SimulateTransactionResult
 from algokit_common.constants import MAX_TRANSACTION_GROUP_SIZE
-from algokit_transact import decode_signed_transaction, encode_signed_transactions, make_empty_transaction_signer
-from algokit_transact.models.signed_transaction import SignedTransaction
+from algokit_transact import make_empty_transaction_signer
+from algokit_transact.codec.signed import decode_signed_transactions
 from algokit_transact.models.transaction import Transaction, TransactionType
 from algokit_transact.ops.fees import calculate_fee
 from algokit_transact.ops.group import group_transactions
 from algokit_transact.ops.ids import get_transaction_id
-from algokit_transact.ops.validate import validate_signed_transaction, validate_transaction
+from algokit_transact.ops.validate import validate_transaction
 from algokit_transact.signer import AddressWithTransactionSigner, TransactionSigner
 from algokit_utils.applications.abi import ABIReturn
 from algokit_utils.applications.app_manager import AppManager
@@ -262,7 +262,7 @@ def __init__(self, params: TransactionComposerParams) -> None:
 
         self._queued: list[_QueuedTransaction] = []
         self._transactions_with_signers: list[TransactionWithSigner] | None = None
-        self._signed_transactions: list[SignedTransaction] | None = None
+        self._signed_transactions: list[bytes] | None = None
         self._raw_built_transactions: list[Transaction] | None = None
 
     def clone(self, composer_config: TransactionComposerConfig | None = None) -> "TransactionComposer":
@@ -459,7 +459,7 @@ def build_transactions(self) -> BuiltTransactions:
         signers = {index: entry.signer for index, entry in enumerate(built_entries) if entry.signer is not None}
         return BuiltTransactions(transactions=transactions, method_calls=method_calls, signers=signers)
 
-    def gather_signatures(self) -> list[SignedTransaction]:
+    def gather_signatures(self) -> list[bytes]:
         self._ensure_built()
         if self._signed_transactions is None:
             self._signed_transactions = self._sign_transactions(self._transactions_with_signers or [])
@@ -503,8 +503,7 @@ def send(self, params: SendParams | None = None) -> SendTransactionComposerResul
 
         # Send transactions and handle network errors
         try:
-            blobs = encode_signed_transactions(signed_transactions)
-            self._algod.send_raw_transaction(blobs)
+            self._algod.send_raw_transaction(signed_transactions)
 
             tx_ids = [get_transaction_id(entry.txn) for entry in self._transactions_with_signers or []]
             group_id = self._group_id()
@@ -627,7 +626,8 @@ def simulate(
                 )
                 for entry in txns_with_signers
             ]
-            signed_transactions = self._sign_transactions(signing_entries)
+            encoded_signed_transactions = self._sign_transactions(signing_entries)
+            signed_transactions = decode_signed_transactions(encoded_signed_transactions)
 
             request = algod_models.SimulateRequest(
                 txn_groups=[algod_models.SimulateRequestTransactionGroup(txns=signed_transactions)],
@@ -813,9 +813,10 @@ def _analyze_group_requirements(  # noqa: C901, PLR0912
             transactions_to_simulate = group_transactions(transactions_to_simulate)
 
         empty_signer: TransactionSigner = make_empty_transaction_signer()
-        signed_transactions = self._sign_transactions(
+        encoded_signed_transactions = self._sign_transactions(
             [TransactionWithSigner(txn=txn, signer=empty_signer) for txn in transactions_to_simulate]
         )
+        signed_transactions = decode_signed_transactions(encoded_signed_transactions)
 
         simulate_request = algod_models.SimulateRequest(
             txn_groups=[algod_models.SimulateRequestTransactionGroup(txns=signed_transactions)],
@@ -1348,7 +1349,7 @@ def _resolve_param_signer(
             return resolved
         return signer
 
-    def _sign_transactions(self, txns_with_signers: Sequence[TransactionWithSigner]) -> list[SignedTransaction]:  # noqa: C901
+    def _sign_transactions(self, txns_with_signers: Sequence[TransactionWithSigner]) -> list[bytes]:
         if not txns_with_signers:
             raise ValueError("No transactions available to sign")
 
@@ -1365,33 +1366,19 @@ def _sign_transactions(self, txns_with_signers: Sequence[TransactionWithSigner])
             blobs = signer(transactions, indexes)
             signed_blobs[key] = list(blobs)
 
-        raw_signed_transactions: list[bytes | None] = [None] * len(transactions)
+        encoded_signed_transactions: list[bytes | None] = [None] * len(transactions)
 
         for key, (_, indexes) in signer_groups.items():
             blobs = signed_blobs[key]
             for blob_index, txn_index in enumerate(indexes):
                 if blob_index < len(blobs):
-                    raw_signed_transactions[txn_index] = blobs[blob_index]
+                    encoded_signed_transactions[txn_index] = blobs[blob_index]
 
-        unsigned_indexes = [i for i, item in enumerate(raw_signed_transactions) if item is None]
+        unsigned_indexes = [i for i, item in enumerate(encoded_signed_transactions) if item is None]
         if unsigned_indexes:
             raise ValueError(f"Transactions at indexes [{', '.join(map(str, unsigned_indexes))}] were not signed")
 
-        # Decode and validate all signed transactions
-        signed_transactions: list[SignedTransaction] = []
-        for index, stxn in enumerate(raw_signed_transactions):
-            if stxn is None:
-                # This shouldn't happen due to the check above, but ensures type safety
-                raise ValueError(f"Transaction at index {index} was not signed")
-
-            try:
-                signed_transaction = decode_signed_transaction(stxn)
-                validate_signed_transaction(signed_transaction)
-                signed_transactions.append(signed_transaction)
-            except Exception as err:
-                raise ValueError(f"Invalid signed transaction at index {index}. {err}") from err
-
-        return signed_transactions
+        return cast(list[bytes], encoded_signed_transactions)  # The guard above ensures no None values
 
     def _group_id(self) -> str | None:
         txns = self._transactions_with_signers or []
@@ -1468,9 +1455,10 @@ def _simulate_error_context(
         """
         try:
             empty_signer: TransactionSigner = make_empty_transaction_signer()
-            signed_transactions = self._sign_transactions(
+            encoded_signed_transactions = self._sign_transactions(
                 [TransactionWithSigner(txn=txn, signer=empty_signer) for txn in sent_transactions]
             )
+            signed_transactions = decode_signed_transactions(encoded_signed_transactions)
             request = algod_models.SimulateRequest(
                 txn_groups=[algod_models.SimulateRequestTransactionGroup(txns=signed_transactions)],
                 allow_empty_signatures=True,

tests/transactions/test_transaction_composer.py

@@ -595,8 +595,7 @@ def test_should_successfully_sign_a_single_transaction(
         signed_txns = composer.gather_signatures()
 
         assert len(signed_txns) == 1
-        assert signed_txns[0] is not None
-        assert signed_txns[0].sig is not None
+        assert len(signed_txns[0]) > 0
 
     def test_should_successfully_sign_multiple_transactions_with_same_signer(
         self, algorand: AlgorandClient, funded_account: AddressWithSigners
@@ -621,8 +620,8 @@ def test_should_successfully_sign_multiple_transactions_with_same_signer(
         signed_txns = composer.gather_signatures()
 
         assert len(signed_txns) == 2
-        assert signed_txns[0].sig is not None
-        assert signed_txns[1].sig is not None
+        assert len(signed_txns[0]) > 0
+        assert len(signed_txns[1]) > 0
 
     def test_should_successfully_sign_transactions_with_multiple_different_signers(
         self, algorand: AlgorandClient, funded_account: AddressWithSigners
@@ -658,8 +657,8 @@ def test_should_successfully_sign_transactions_with_multiple_different_signers(
         signed_txns = composer.gather_signatures()
 
         assert len(signed_txns) == 2
-        assert signed_txns[0].sig is not None
-        assert signed_txns[1].sig is not None
+        assert len(signed_txns[0]) > 0
+        assert len(signed_txns[1]) > 0
 
     def test_should_throw_error_when_no_transactions_to_sign(self, algorand: AlgorandClient) -> None:
         """Test that an error is thrown when there are no transactions to sign."""
@@ -754,29 +753,3 @@ def faulty_signer(_txns: Sequence[Transaction], _indexes: Sequence[int]) -> list
 
         with pytest.raises(ValueError, match=r"Transactions at indexes \[0\] were not signed"):
             composer.gather_signatures()
-
-    def test_should_throw_error_when_signer_returns_invalid_signed_transaction_data(
-        self, algorand: AlgorandClient, funded_account: AddressWithSigners
-    ) -> None:
-        """Test error handling when a signer returns malformed signed transaction data."""
-        from collections.abc import Sequence
-
-        from algokit_transact.models.transaction import Transaction
-
-        # Create a faulty signer that returns invalid data
-        def faulty_signer(_txns: Sequence[Transaction], indexes: Sequence[int]) -> list[bytes]:
-            return [bytes([1, 2, 3]) for _ in indexes]
-
-        composer = algorand.new_group()
-        composer.add_payment(
-            PaymentParams(
-                sender=funded_account.addr,
-                receiver=funded_account.addr,
-                amount=AlgoAmount.from_micro_algo(1000),
-                signer=faulty_signer,
-            )
-        )
-
-        # Should provide a clear error message indicating which transaction had invalid data
-        with pytest.raises(ValueError, match="Invalid signed transaction at index 0"):
-            composer.gather_signatures()