Fix Sign Transaction Signature

[MiguelLZPF]

Description

Changed the transaction bytes to be signed. More details available in the issue.

Fixes #124

[franfernandez20]

I recommend resolving this issue by reverting the implementation to Transaction.toBytes() and Transaction.fromBytes(). This approach not only resolves the problem but also eliminates the need to use private SDK methods. Additionally, it restores the capability to set multiple nodeIds.
Consequently, we should update the definition in the WalletConnect documentation for hedera_signTransaction to reflect these changes.

[bugbytesinc]

The multiple node id concept pulls against the intent of the wallet connect infrastructure, it is the source of other bugs and potential future exploits. Also, supporting multiple transactions in this could break conformity with HIP-820. The private methods are used because part of the constraints when creating the library was that "the JS sdk could not be changed in any way".

[bugbytesinc]

The real fix is to fix the JS SDK first, so it can support the proper wallet connect functionality.

[gregscullard]

Would it make sense to retain the existing method and add support for .toBytes serialised transactions as an additional method for backwards compatibility ? A decision to deprecate (or not) the current implementation can be made in the future.

The multiple node id concept pulls against the intent of the wallet connect infrastructure

How is this the case ? Wallet connect establishes a secure tunnel between a dApp and a wallet, it doesn't (to my knowledge) prescribe what content can or can't be transported over the protocol. Wallet connect became chain agnostic for this very reason (it used to prescribe eth transactions only).

it is the source of other bugs and potential future exploits.

to/fromBytes is used extensively, if there are bugs they should be fixed. If a dApp is concerned over the use of multiple nodes, it can choose to specify a single one or delegate the node selection to the wallet per https://hips.hedera.com/hip/hip-745 or continue using the current method (subject to deprecation decision per comment above)

Also, supporting multiple transactions in this could break conformity with HIP-820.

Standards aren't meant to be static, HIP-820 can be augmented to include this alternative method.

The private methods are used because part of the constraints when creating the library was that "the JS sdk could not be changed in any way".

The SDK couldn't be changed in time for the first release of wallet connect, it can of course be changed to meet the needs of the developer community.

[gregscullard]

I recommend resolving this issue by reverting the implementation to Transaction.toBytes() and Transaction.fromBytes().

Given this PR suggests a change to how a transactionBody is "extracted" from a Transaction, suggest we keep the conversation here relevant to this change and discuss support for to/fromBytes separately.

The prior implementation extracted a transactionBody from the Transaction and created a new body using a nodeAccountId that was selected at random by the library, thereby changing the transaction body prior to requesting a signature.

For example: If a developer created a transaction using the SDK, specified exactly one node he/she wanted to use, the library arbitrarily selected a different node (it could be the same thanks to randomness, but no guarantee)
The proposed change remains faithful to the developer's node selection.

[gregscullard]

I'm uncomfortable changing the signature of a public method, this could result in a breaking change to existing implementations.

[gregscullard]

Maybe this implementation should remain and Signer.ts changed from

    const transactionBody: proto.TransactionBody = transactionToTransactionBody(
      transaction,
      this._getRandomNodes(1)[0],
    )

to re-using the nodeAccountId from transaction (if that's possible) ?

This would no longer be a breaking change.

[MiguelLZPF]

Thanks for the feedback! 🙂

From our testing and research, the method used in this PR is the only way to get the signed transaction validated by the Hedera network. The previous approach signed the transaction in a way that the Hedera Network couldn't verify.

That said, we can leave the nodeAccountId parameter for compatibility, although it's currently unused and likely not functional in its previous form. @gregscullard, should I include the nodeAccountId as you suggested?

If there is a better method to obtain the signed transaction bytes than the one used in this PR, please let me know. Based on our testing a few weeks ago, this was the only approach that worked.

[gregscullard]

I'll leave it to the community of builders/users to determine whether a breaking change in the signature of the transaction is acceptable or not. The change is minor and would likely be implemented as a result of changes to the application, so tested and working before deployed. Breaking but not critically so I guess.

[tmctl]

added another comment about accessing _signedTransactions, would the transaction body be in that array before a transaction is signed?

[tmctl]

This utility function is used to get the transaction body before the transaction is signed, based on the name of _signedTransactions, I'd imagine this internal function only has the transactions in this array if the transaction has already been signed?

If so, we may need to try another method

@MiguelLZPF

[MiguelLZPF]

Hi @tmctl! if I understood you correctly, there is a problem getting the signed transaction body here because this utility function was intended to get the transaction body not the signed transaction body.
So, the original issue is that HederaWalletConnect is getting the transaction body also when it should be looking for the signed transaction body.
So the solution is to have two separate utility functions then, right?

[MiguelLZPF]

Added in the latest commit, take a look at it and tell me what you guys think 🤔

[MiguelLZPF]

I've reverted the change; the issue was with the naming. transaction._signedTransactions.current.bodyBytes actually contains the UNSIGNED transaction body bytes, which makes sense since this occurs before the transaction is signed. It was confusing initially because of the misleading name, as you pointed out in your previous message.

[MiguelLZPF]

This is the signed transaction bytes: signedTransaction._transactions.current.signedTransactionBytes 🫣. In my opinion is mega-difficult to understand this structure...

[MiguelLZPF]

Just to add more clarity about this topic, this are our testing results for only sign Hedera SDK Transaction Objects:

  const fullTxBytes = transaction.toBytes();
  const transactionBytes = transaction._signedTransactions.get(0)!.bodyBytes!; //* <--
  const signedTransactionBytes = signedTransaction._transactions.get(0)!.signedTransactionBytes!;
  const signatures = signedTransaction.getSignatures().get(newAccountId)!;
  const transactionHash = await signedTransaction.getTransactionHash();
console.log(`
    Full Transaction: ${uint8ArrayToHex(fullTxBytes)}
    Transaction: ${uint8ArrayToHex(transactionBytes)}
    Signed Transaction: ${uint8ArrayToHex(signedTransactionBytes)}
    Tx Hash: ${uint8ArrayToHex(transactionHash)}
    Signature: ${uint8ArrayToHex(signatures.get(privateKey.publicKey)!)
')

[gregscullard]

LGTM