Update DAppSigner.ts

[teacoat]

When working with HSuite they were encountering issues with signing transactions, this is the change we applied to hashconnect in order to fix the issue. When signing a transaction multiple times (multisig), you want to preserve the node id.

May resolve this: #124

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🔴	Statements	45.26% (-0.24% 🔻)	258/570
🔴	Branches	25.14% (-0.59% 🔻)	44/175
🔴	Functions	27.34%	38/139
🔴	Lines	45.4% (-0.26% 🔻)	242/533

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🔴	... / DAppSigner.ts	47.5% (-1.85% 🔻)	13.64% (-1.36% 🔻)	37.5%	47.37% (-1.95% 🔻)

Test suite run success

25 tests passing in 2 suites.

Report generated by 🧪jest coverage report action from 17cad0d

[teacoat]

It is possible the entire signing function here is wrong, I will do some more testing on this as its quite different to what we are doing in hashconnect

[franfernandez20]

Maybe we can use an approach similar to this function: keep setting the nodes if the transaction does not have any set, and use the node from the transaction if it does.

/**
 * Sets default consensus nodes that a transaction will be submitted to. Node Account ID(s)
 * must be set before a transaction can be frozen. If they have already been set, this
 * function will not modify the transaction.
 * @param transaction - any instance of a class that extends `Transaction`
 *
 * @see {@link https://docs.hedera.com/hedera/networks/testnet/testnet-nodes | Full list of Testnet-nodes}
 * @see {@link https://docs.hedera.com/hedera/networks/mainnet/mainnet-nodes | Full list of Mainnet-nodes}
 */
export function setDefaultNodeAccountIds<T extends Transaction>(transaction: T): void {
  const isNodeAccountIdNotSet =
    !transaction.nodeAccountIds || transaction.nodeAccountIds.length === 0

  if (!transaction.isFrozen() && isNodeAccountIdNotSet)
    transaction.setNodeAccountIds([new AccountId(3), new AccountId(4), new AccountId(5)])
}

[tomachianura]

thanks guys,trule appreciated!

[gregscullard]

LGTM

[tmctl]

@franfernandez20 this changes the original suggested change approved by @gregscullard here: d47e8a7

It might be helpful in the future to do a PR into another branch that has an open PR so that we can better see the diff.

I've added some notes here - https://github.com/hashgraph/hedera-wallet-connect/pull/213/files

Fran's updates effectively check to see if the transaction has nodeAccountIds and then uses the first one, otherwise uses the internal function to get a random node account id. This logic makes sense to me.

[tmctl]

github action annotations might make it harder to read the diff in this PR, if you press a on the files changed page, it is a quick way to toggle the annotations.

[hgraphql]

LGTM