Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ feat(gentest): Support for transaction types #1166

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

raxhvl
Copy link
Contributor

@raxhvl raxhvl commented Feb 2, 2025

πŸ—’οΈ Description

Support for type 1 and 2 transaction.

I have not created a test case to test type 1 transaction. Let me know if that is okay.

πŸ”— Related Issues

closes #861

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.

@raxhvl
Copy link
Contributor Author

raxhvl commented Feb 3, 2025

While working on this issue I realized that we have two models to represent a Transaction:

  1. Transaction used by tests

class TransactionGeneric(BaseModel, Generic[NumberBoundTypeVar]):
"""
Generic transaction type used as a parent for Transaction and
FixtureTransaction (blockchain).
"""
ty: NumberBoundTypeVar = Field(0, alias="type") # type: ignore
chain_id: NumberBoundTypeVar = Field(default_factory=lambda: TransactionDefaults.chain_id) # type: ignore
nonce: NumberBoundTypeVar = Field(0) # type: ignore
gas_price: NumberBoundTypeVar | None = None
max_priority_fee_per_gas: NumberBoundTypeVar | None = None
max_fee_per_gas: NumberBoundTypeVar | None = None
gas_limit: NumberBoundTypeVar = Field(21_000) # type: ignore
to: Address | None = None
value: NumberBoundTypeVar = Field(0) # type: ignore
data: Bytes = Field(Bytes(b""))
access_list: List[AccessList] | None = None
max_fee_per_blob_gas: NumberBoundTypeVar | None = None
blob_versioned_hashes: Sequence[Hash] | None = None
v: NumberBoundTypeVar | None = None
r: NumberBoundTypeVar | None = None
s: NumberBoundTypeVar | None = None
sender: EOA | None = None

  1. Transaction received from an RPC

class TransactionByHashResponse(CamelModel):
"""Represents the response of a transaction by hash request."""
block_hash: Hash | None = None
block_number: HexNumber | None = None
transaction_hash: Hash = Field(..., alias="hash")
from_address: Address = Field(..., alias="from")
to_address: Address | None = Field(..., alias="to")
ty: HexNumber = Field(..., alias="type")
gas_limit: HexNumber = Field(..., alias="gas")
gas_price: HexNumber | None = None
max_fee_per_gas: HexNumber | None = None
max_priority_fee_per_gas: HexNumber | None = None
value: HexNumber
data: Bytes = Field(..., alias="input")
nonce: HexNumber
v: HexNumber
r: HexNumber
s: HexNumber

We should combine them.

@marioevz
Copy link
Member

marioevz commented Feb 3, 2025

We should combine them.

There are slight differences but I agree we could minimize code duplication by at least inheriting Transaction from TransactionByHashResponse.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Thanks! Some small comments.

assert (
res.ty == 0
), f"Transaction has type {res.ty}: Currently only type 0 transactions are supported."
assert res.ty < 4, f"Unsupported transaction type :{res.ty}"
Copy link
Member

Choose a reason for hiding this comment

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

I think this implies we support type 3 transactions too, and the only missing thing would be the versioned hashes.

transaction.gas_price = None
transaction.max_fee_per_gas = res.max_fee_per_gas
transaction.max_priority_fee_per_gas = res.max_priority_fee_per_gas

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if res.ty == 3:

I think we can add the blob versioned hashes here without much issues.

@raxhvl
Copy link
Contributor Author

raxhvl commented Feb 6, 2025

I explored ways to make Gentest more robust. In our codebase, the Transaction model is responsible for defining new transaction types. The TransactionResponseByHash is the RPC equivalent of a Transaction, with added post-execution metadata like the transaction hash, block number etc.

Previously, Gentest had its own Transaction (3rd!) model that performed separate validation. This redundant code essentially dealt with the same entity, leading to unnecessary complexity.

I have refactored the code to make the Transaction model from ethereum_test_types the source of truth. This model has comprehensive validation logic that handles most use cases. The RPC transaction now extends this model, ensuring that RPC transactions are automatically validated. It is possible we face some short term issues due to this change in our code using RPC data. I have tested and have not encountered any problems yet.

The primary takeaway of this exercise is that Gentest will now automatically support both current and future transactions as long as the root Transaction model is kept up to date.

classDiagram
    Transaction <|-- TransactionByHashResponse : extends
    TransactionByHashResponse <|-- Gentest : consumes
    class Transaction{
      +..tx fields..
      +validate()
    }

    class TransactionByHashResponse{
        + transaction_hash
        + block_number
        ....
    }
Loading

@raxhvl raxhvl changed the title ✨ feat(gentest): Type 1 and 2 transaction ✨ feat(gentest): Support for transaction types Feb 6, 2025
@marioevz
Copy link
Member

marioevz commented Feb 6, 2025

So I just tried out uv run execute with a type 4 test and it's currently failing when parsing this:

{
  "type": "0x4",
  "chainId": "0x1",
  "nonce": "0x0",
  "gas": "0x7a120",
  "maxFeePerGas": "0x3b9aca00",
  "maxPriorityFeePerGas": "0x3b9aca00",
  "to": "0xd69dcd96dce63dddc8921cc4efc8b8027223084e",
  "value": "0x0",
  "accessList": [],
  "authorizationList": [
    {
      "chainId": "0x0",
      "address": "0x2da5fa1a13279ff30b734d02370058ce60e3c478",
      "nonce": "0x0",
      "yParity": "0x0",
      "r": "0x18fc889a2b9cf91b2c57bfa5359f1d1150aefbaf474daa418a3e720dff1ff52b",
      "s": "0xa570fceb4c95d81d8d308075111274588402b0931b680d58ca295c466a9d5cb"
    }
  ],
  "input": "0x",
  "r": "0x1292b87437eb92c88584e91dfa2dd7f1e0ee46823b0afec04d1b2e4e727122d9",
  "s": "0x772385b2de94a80c5e0ddd0d991c2999532e7f75ef1047ffcd66e04ee5f39fd1",
  "yParity": "0x0",
  "v": "0x0",
  "hash": "0x2dd9536121dead91dc6e738518aef5fe4f328d3e909a02a5886586ef5c166899",
  "blockHash": "0xfac2310773979fc4760c1564f01a78dfca6c499e7d74a6f553effca00ee768c4",
  "blockNumber": "0x4",
  "transactionIndex": "0x0",
  "from": "0xe5d78afb66042679f59c579d2212bbf5ea0f19ae",
  "gasPrice": "0x3b9aca00"
}

I'm not sure why would it give me gasPrice alongside maxFeePerGas and maxPriorityFeePerGas, but I still need to check other clients to see if this is common behavior.

We can add a simple preprocessor to the pydantic model to remove this redundant field if it's present.

@marioevz
Copy link
Member

marioevz commented Feb 6, 2025

Same thing for geth it seems:

{
  "blockHash": "0x7beb4ddaa4bb1db2db4d74d2c5ab4ad089b41db2b99fa340a99f8bd04cbb43d7",
  "blockNumber": "0x4",
  "from": "0x02d9cd73d16fdddb6fff1c7e2b7ce8453c01c25b",
  "gas": "0x7a120",
  "gasPrice": "0x3b9aca00",
  "maxFeePerGas": "0x3b9aca00",
  "maxPriorityFeePerGas": "0x3b9aca00",
  "hash": "0x536308c49c82a5882c05af3b0d274def18e21b05a6167fb534e6c8a31472954a",
  "input": "0x",
  "nonce": "0x0",
  "to": "0xb3e1ad1b45d1afea8a30fd97aa1ea1f640b45d2e",
  "transactionIndex": "0x0",
  "value": "0x0",
  "type": "0x4",
  "accessList": [],
  "chainId": "0x1",
  "authorizationList": [
    {
      "chainId": "0x0",
      "address": "0x675cf18f4594fc7725af95a6a57cd91e8f05d560",
      "nonce": "0x0",
      "yParity": "0x1",
      "r": "0x4f29c40cad8b8e1a0ab00869c562fbe9be2981ceaad14d9e7fc54ca66c2f2c1",
      "s": "0x15550dc1ea9980796c1476514b00cd065a46a67ab26a390cae2b7cfab9562df0"
    }
  ],
  "v": "0x0",
  "r": "0xa827f534e880af462373ba765b561c2bd83a27e4b6e5ee6f59c4cb542e1266a9",
  "s": "0x64b4db8ba237a655665bd3929b6e41202f1e76d10fa4cc8d5b7ed6a7bb2312ba",
  "yParity": "0x0"
}

I think we can simply check if maxFeePerGas is present, and if so, remove gasPrice.

@marioevz
Copy link
Member

marioevz commented Feb 6, 2025

I've tested the execute command with some type-4 tests, tried all clients on raxhvl#2, and only Besu is currently not passing because of how they are formatting the authorization lists, so I think we should be ok with merging that PR and then this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨(gentest): add support for other transaction types
2 participants