VG-16377 handle transaction with no recipient

[flocks]

📝 Description

Some transaction don't have any recipient. For instance, using remix IDE you can trigger a transaction to deploy a smart contract, it will generate a transaction without a to field.

This is the quickest solution, perhaps not the cleanest. We just build an EthereumTransaction with an empty string in the recipient field and let the consumer of the live app handle this case.

The other solution would be to change wallet-api types..etc.., but not sure it is worth it.

Tested on the vault with no issue, not sure what are the impacts for ledger-live though.

❓ Context

• Linked resource(s): VG-16377

📸 Demo

🚀 Expectations to reach

Pull Requests must pass the CI and be internally validated in order to be merged.

[cgrellard-ledger]

Looks good to me but as I’m not sure of the impact of this on Ledger Live maybe a review of someone from the blockchain team could be beneficial

[flocks]

At first glance this won’t work out of the box on LL since LL does not handle tx without recipient.

of course, but it's already NOT working on the Live. If you use walletconnect on LL and try to deploy a contract with remix, it will fail before reaching LL, because walletconnect-live-app does not expect to receive a transaction without to and as a result you will have a runtime error

[jeanmichel-ledger]

At first glance this won’t work out of the box on LL since LL does not handle tx without recipient.

Can you provide more input on what is expected to unblock merge ?

[chabroA]

At first glance this won’t work out of the box on LL since LL does not handle tx without recipient.

Can you provide more input on what is expected to unblock merge ?

Depends on what this PR goal is I guess (PS: I have no other context that is PR description)

Based on @flocks comment (https://github.com/LedgerHQ/wallet-connect-live-app/pull/113#issuecomment-1702939318) it looks like the only goal of this PR is to prevent the live app from crashing when not receiving an address for a tx.

I initially though (maybe wrongly) that the goal of this PR was to properly "handle" (sign and broadcast) such tx.

My review comment only covers the following part of the desc:

not sure what are the impacts for ledger-live though.

By "At first glance this won’t work out of the box on LL since LL does not handle tx without recipient." I mean you won't be able to sign a tx without a recipient, since this logic is not handled on LL as of today. Now, if this is not the expected goal of this PR (to ba able to sign and broadcast tx without recipient like a contract creation tx), the only impact on LL of such change should be an error in the "Sign transaction" flow warning the user that the tx can't be signed since it does not have a recipient (cf. this line).

Lastly, to clear any doubts you might have regarding impacts on LL, the best thing to do would be to quickly test your PR in LL context (cf. loading a local manifest https://developers.ledger.com/docs/live-app/developer-mode/#add-a-local-app).

With that said, removing my review to let you handle it as you see fit

[flocks]

Based on @flocks comment (https://github.com/LedgerHQ/wallet-connect-live-app/pull/113#issuecomment-1702939318) it looks like the only goal of this PR is to prevent the live app from crashing when not receiving an address for a tx.

Yes! The goal is just to prevent wc-liveapp from crashing when there is no to field in the transaction it receives.

On the vault, we assume the transaction is a deploy contract transaction when we receive an empty string in the recipient field.

[drakoFukayu]

👋 I believe we clear the unknown on the objective of this PR aka: (1) it prevent the live app to crash on a smart contract deployment from the Vault which is using this live app (2) no impact in LL, as this PR goal is not to allowed user to deploy smart contract deployment in LL Could we get some approvals and merge? If not what are we missing ?

[ghost]

Hi team, sorry for the lag but just to clarify: I think both this repo and the scope of the PR are related to @LedgerHQ/wallet-api, team blockchain isn't relevant for this review. @ComradeAERGO FYI.

[ComradeAERGO]

@haammar-ledger Hi and thanks for letting us know about this.

Unfortunately, this repo is not into our scope. A solution we could handle would be to change our tx definition in Wallet API to support tx without "to" fields.

The solution produced here could work for sure, but we have no ownership on this repo.

cc @Justkant @adrienlacombe-ledger

[flocks]

I approve but could you remove the ternary operator, please? recipient: ethTx.to ? eip55.encode(ethTx.to) : "", and replace with recipient: eip55.encode(ethTx.to) As it comes from our mock and it will always be defined. It wrongly suggests that it can be undefined. Thanks

Yes but recipient: eip55.encode(ethTx.to) will not pass typecheck, as now to is optional in ethTx. Even if it comes from mocks and we know to is there, it still has EthTransaction type

[KVNLS]

I approve but could you remove the ternary operator, please? recipient: ethTx.to ? eip55.encode(ethTx.to) : "", and replace with recipient: eip55.encode(ethTx.to) As it comes from our mock and it will always be defined. It wrongly suggests that it can be undefined. Thanks

Yes but recipient: eip55.encode(ethTx.to) will not pass typecheck, as now to is optional in ethTx. Even if it comes from mocks and we know to is there, it still has EthTransaction type

Ok, I understand your point. Good to me, thanks for the clarification!

[flocks]

If you prefer, instead of updating EthTransaction, we could do something like:

export type EthTransaction = {
  value: string
  to: string
  gasPrice: string
  gas: string
  data: string
}
export type EthTransactionContractDeploy = {
  value: string
  gasPrice: string
  gas: string
  data: string
}

export function convertEthToLiveTX(
  ethTX: EthTransaction | EthTransactionContract,
): EthereumTransaction {
  return {
    family: "ethereum",
    amount:
      ethTX.value !== undefined
        ? new BigNumber(ethTX.value.replace("0x", ""), 16)
        : new BigNumber(0),
    recipient: "to" in ethTX ? eip55.encode(ethTX.to) : "",
    gasPrice:
      ethTX.gasPrice !== undefined
        ? new BigNumber(ethTX.gasPrice.replace("0x", ""), 16)
        : undefined,
    gasLimit: ethTX.gas !== undefined ? new BigNumber(ethTX.gas.replace("0x", ""), 16) : undefined,
    data: ethTX.data ? Buffer.from(ethTX.data.replace("0x", ""), "hex") : undefined,
  }
}

Then we can remove the ternary in the test because we know it's EthTransaction (that now have always a to) and not a EthTransactionContractDeploy