"hedera_signTransaction" method broke prev signed transactions

[franfernandez20]

Describe the bug A clear and concise description of what the bug is.

The example demonstrating how to sign and return a transaction is preventing the correct use of a previously signed transaction. The transaction signers publicKeys and sigMaps are loosed when converting the incoming transaction with _makeTransactionBody. If you send a signed transaction to be signed when you receive it has been changed.

To Reproduce Steps to reproduce the behaviour:

Here starts the actual flow of hedera_signTransaction in the examples:

https://github.com/hashgraph/hedera-wallet-connect/blob/1cbfef6e647e6a26f3d298acbfedf19286eab964/src/examples/typescript/dapp/main.ts#L235C7-L235C35

The returned signatureMap cannot recreate the original transaction with the original signatures plus the new one.

This is a summary of the code involved to reproduce the problem:

const client = Client.forNetwork("testnet").setOperator("<accountA>", "<pk>");

const transactionId = TransactionId.generate(AccountId.fromString("<accountA>"));

const tranferTransaction = new TransferTransaction()
  .setTransactionId(transactionId)
  .addHbarTransfer("<accountB>", Hbar.fromTinybars(1).negated())
  .addHbarTransfer("0.0.8000", Hbar.fromTinybars(1))
  .freezeWith(client)

const transaction = await tranferTransaction.signWithOperator(client)

const trxBody = transaction._makeTransactionBody(AccountId.fromString("0.0.3"))
const encodedBody = proto.TransactionBody.encode(trxBody).finish()
const transactionBody = Buffer.from(encodedBody).toString('base64')

/** 
 * Now the transaction is sended through wallet connect messaging protocol
 */

const signer = new Wallet("<accountB", "<pk>")

// transactionBody is received from the other side of the wallet connect
const body = Buffer.from(transactionBody, 'base64')

const signerSignatures = await signer.sign([body])

const _signatureMap = proto.SignatureMap.create(
  signerSignaturesToSignatureMap(signerSignatures),
)

const signatureMap = signatureMapToBase64String(_signatureMap)

/**
 * Now the signatureMap is sent back
 * and we try to restore the transaction with the signatureMap
 */

const sigMap = base64StringToSignatureMap(signatureMap)
const bodyBytes = Buffer.from(transactionBody, 'base64')

const bytes = proto.Transaction.encode({ bodyBytes, sigMap }).finish()
const restoredTransaction = Transaction.fromBytes(bytes)

Expected behavior A clear and concise description of what you expected to happen.

I expected the wallet to keep the previous signs

Desktop (please complete the following information):

• OS: web/Node
• Browser Chrome
• Version 123.0.6312.59 / v18.18.0

[franfernandez20]

I attached an open issue in the hedera-sdk-js where it is suggested not to use this method for serializing/deserializing transactions.

https://github.com/hashgraph/hedera-sdk-js/issues/2218

[gregscullard]

As far as I know, this is the expected behaviour for _makeTransactionBody which only generates a transaction body and doesn't include any signatures, signatures have to be added as part of const bytes = proto.Transaction.encode({ bodyBytes, sigMap }).finish()

Either the dApp needs to keep hold of the original signature maps and merge the signature map from wallet connect before calling the line above, or wallet connect needs to support an alternative (toBytes, fromBytes).

I'm aware of prior concerns with fromBytes and am working with the sdk team to resolve.

Note: this issue only concerns use cases where a transaction that was previously signed by other parties requires an additional signature from wallet connect.

Note: in testing the merging of sigMaps idea, the bodyBytes resulting from Buffer.from(transactionBody, 'base64') didn't verify signature against the original signature from the tranferTransaction object.

[gregscullard]

_makeTransactionBody also precludes the ability for wallet connect to support transactions that are setup for multiple nodes (as is usually the case), this may lead the end user to have to re-sign transactions multiple times in the event of nodes not being available which is detrimental to UX.

[franfernandez20]

It's crucial we find a solution, especially because we all approve this implementation. I tag all the people who request to be a reviewer.

@rajesuwerps @bugbytesinc @justynspooner @hgraphql @teacoat @valeraOlexienko

[bugbytesinc]

Unfortunately, the nature of the underlying official SDKs tear apart the protobuf regularly and re-constitutes it. Even rebuilding it multiple times depending on the context. The WC implementation needs to be careful and guard the "fist version" of the protobuf serialization as the source of truth. The nature of the design of the underlying hedera sdks indeed makes this difficult.