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

Refactor TransactionForRpc #7446

Closed
wants to merge 78 commits into from

Conversation

emlautarom1
Copy link
Contributor

Partially solves #7313

Changes

  • Remove TransactionForRpc
  • Introduce RpcGenericTransaction only for deserializing RPC transactions.
  • Introduce a proper type hierarchy based on RpcNethermindTransaction and individual transaction types (Legacy, AccessList, EIP1559, Blob and Deposit), with support for extension.
  • Introduce conversions between all types.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

Unfortunately a very long PR due to the introduction of several changes. Currently, we have a single class used for both serialization and deserialization:

This class does everything, from JSON (de)serializing based on reflection, to conversion from/to our main Transaction type. This is cumbersome and makes extending the possible transaction types very brittle (see https://github.com/NethermindEth/nethermind/blob/c6a768d2f2f80fe09f2e711d502e0c8347bd9e02/src/Nethermind/Nethermind.Optimism/Rpc/OptimismTransactionForRpc.cs).

This PR separates these responsibilities:

When deserializing:

When serializing

  • We have multiple Converters that know how to take a Transaction and build a RpcNethermindTransaction, which is a base class for all JSON-RPC Transaction types. We enrich this type with some Nethermind specific data not included in the spec (ex. transaction hash). Each converter knows how to take a Transaction with a specific TxType and build a subclass (ex. there is a converter from Transaction to RpcBlobTransaction). Each converter is again based on the Ethereum JSON-RPC spec to ensure no fields are missing. Again, we use the same registry approach.
  • To write JSON, we instruct the serializer to use the runtime type rather than the static type, that is, don't use RpcNethermindTransaction but, for example, RpcEIP1559Transaction.

The main difference with the original design is the separation of input from output Transactions. Initially I tried to use the same classes for both (ex. (de)serialize RpcLegacyTransaction just using the TxType value) but this quickly becomes a mess since input fields might be optional but required when serializing and we have two defaulting mechanism, C# and Ethereum (ex. Address? SenderAddress defaults to null by the type system, but we might want to default it to a different value like Address.SystemUser).

Lastly, this PR is in draft and suggestions are welcome.

@emlautarom1
Copy link
Contributor Author

We'll move forward with the alternative design (#7483)

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.

1 participant