Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Conversation

@Spacesai1or
Copy link
Contributor

@Spacesai1or Spacesai1or commented Aug 4, 2022

Before this PR, web3Eth.signTransaction was returning whatever the user passed as the transaction parameter for respnose.tx when the RPC client was returning only the RLP encoded transaction as the RPC response (in the below code example, Ganache was only returning the value labeled as raw, while Geth was returning the entire object (i.e. { raw: ..., tx: ... })). This resulted in varying returns values based on which client the request was submitted to:

If the connected client was Ganache:

{
    raw: '0xf86580843b9aca018252089400000000000000000000000000000000000000000180820a95a02bd3ccac498541156149591091ff2af043f0be204a7d723701a9e1366dcc6f5fa02a138f18dcafebcf48d9df2c7b3d7fa5f0741b2921d6f42fdc219f34a7ddd97e',
    tx: {
      from: '0x6e599da0bff7a6598ac1224e4985430bf16458a4',
      nonce: 0n,
      to: '0x0000000000000000000000000000000000000000',
      value: 1n,
      gas: 21000n,
      gasPrice: 1000000001n
    }
}

If the connected client was Geth:

{
   raw: '0xf86580843b9aca018252089400000000000000000000000000000000000000000180820a95a003d7879059608586185d1f1b662a73f2701026403b44ab639b5558b043ca375ca0024054e057f5cc1e1f3d2c7aabf162616caf95bbcfb8215376210ff410d91f32',
   tx: {
      type: 0n,
      nonce: 0n,
      gasPrice: 1000000001n,
      gas: 21000n,
      value: 1n,
      v: 2709n,
      r: '0x03d7879059608586185d1f1b662a73f2701026403b44ab639b5558b043ca375c',
      s: '0x024054e057f5cc1e1f3d2c7aabf162616caf95bbcfb8215376210ff410d91f32',
      to: '0x0000000000000000000000000000000000000000',
      data: '0x'
    }
}

After this PR:

If the connected client was Ganache:

{
    raw: '0xf86580843b9aca018252089400000000000000000000000000000000000000000180820a95a02bd3ccac498541156149591091ff2af043f0be204a7d723701a9e1366dcc6f5fa02a138f18dcafebcf48d9df2c7b3d7fa5f0741b2921d6f42fdc219f34a7ddd97e',
    tx: {
      nonce: 0n,
      gasPrice: 1000000001n,
      gasLimit: 21000n,
      to: '0x0000000000000000000000000000000000000000',
      value: 1n,
      data: '0x',
      v: 2709n,
      r: '0x2bd3ccac498541156149591091ff2af043f0be204a7d723701a9e1366dcc6f5f',
      s: '0x2a138f18dcafebcf48d9df2c7b3d7fa5f0741b2921d6f42fdc219f34a7ddd97e',
      type: 0n
    }
}

If the connected client was Geth:

{
    raw: '0xf86580843b9aca018252089400000000000000000000000000000000000000000180820a95a0e7e229dcf3b6930ec7a090164f3bafe42fe4b343e2ac89f4e7077992e4cae7a9a0368c25b3275b8f8d8c9ea7b4fd7715b474843f4a6f36c9088d1c9e3f0d61dc27',
    tx: {
      type: 0n,
      nonce: 0n,
      gasPrice: 1000000001n,
      gas: 21000n,
      value: 1n,
      v: 2709n,
      r: '0xe7e229dcf3b6930ec7a090164f3bafe42fe4b343e2ac89f4e7077992e4cae7a9',
      s: '0x368c25b3275b8f8d8c9ea7b4fd7715b474843f4a6f36c9088d1c9e3f0d61dc27',
      to: '0x0000000000000000000000000000000000000000',
      data: '0x'
    }
}

One point of discussion that remains: In the last two above code examples, the response from Ganache returns the gasLimit property because the raw encoded transaction is being parsed by @ethereumjs/tx's TransactionFactory. However, the response from Geth returns gasLimit as gas which is what's being done in 1.x. The decision to be made it whether we will continue to use gas to describe gasLimit in 4.x

  • Both the old and new ETH RPC spec use gas for eth_sendTransaction
  • Ethers.js uses gasLimit

closes #5077

@render
Copy link

render bot commented Aug 4, 2022

@Spacesai1or Spacesai1or self-assigned this Aug 4, 2022
@Spacesai1or Spacesai1or added the 4.x 4.0 related label Aug 4, 2022
@Spacesai1or Spacesai1or marked this pull request as ready for review August 4, 2022 17:55
value: '0x174876e800',
gas: '0x5208',
gasPrice: '0x4a817c800',
type: '0x0',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add tests for 0x1 and 0x2 transactions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated via this commit

* @param returnFormat ({@link DataFormat} Specifies how the return data should be formatted.
* @returns {@link SignedTransactionInfoAPI}, an object containing the RLP encoded signed transaction (accessed via the `raw` property) and the signed transaction object (accessed via the `tx` property).
*/
export function decodeSignedTransaction<ReturnFormat extends DataFormat>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this support EIP-2718 Enveloped transactions decoding? I think not, Its only for RLP encoded 0x0 tx decoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it can handle type transactions as shown by tests still passing after this commit. This is because transaction parsing is done via @ethereumjs/tx's TransactionFactory.fromSerializedData here

@jdevcs
Copy link
Contributor

jdevcs commented Aug 9, 2022

The decision to be made it whether we will continue to use gas to describe gasLimit in 4.x
Both the old and new ETH RPC spec use gas for eth_sendTransaction
Ethers.js uses gasLimit

Yellow paper specifies gasLimit instead of gas, but EL specs is using gas. ethereum/execution-apis#283 will see its response.

IMO we should keep gas field for now. and in future should add support of gasLimit field as well

@Muhammad-Altabba
Copy link
Contributor

Is it good to have both gasLimit and gas. Where both of them will have the exact same value? This would mean that the library user can read any of them. And this means, for the user, for example, an easier migration from ethers.js.

Copy link
Contributor

@nikoulai nikoulai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from @jdevcs comments, LGTM.

I think we should go with gasLimit as users will be more familiar with yellow paper than the execution specs and would, also, make migration from Ethers.js easier. But, would like to hear the rest of the team.

@Spacesai1or
Copy link
Contributor Author

Spacesai1or commented Aug 26, 2022

I added code to replace gasLimit with gas when calling formatTransaction thinking this would be a good way to enforce the library only returning gas instead of gasLimit or gas, however this broke a lot of unit tests and wasn't a complete solution as there are other instances where transaction objects are formatted that don't use formatTransaction such as getTransaction. So, I propose we make a separate issue to decide globally how the library should handle gasLimit vs gas as a lot of code would need to be updated that's not directly related to this PR


#5387 was created to track this

@Spacesai1or Spacesai1or mentioned this pull request Aug 26, 2022
@Spacesai1or Spacesai1or requested a review from jdevcs August 26, 2022 02:44
@Spacesai1or Spacesai1or merged commit e2afb75 into 4.x Aug 30, 2022
@Spacesai1or Spacesai1or deleted the wyatt/4.x/5077-sign-transaction-geth-fix branch August 30, 2022 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

4.x 4.0 related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants