Refactor `TransactionForRpc` (Alternative) #7483

emlautarom1 · 2024-09-23T18:50:29Z

Partially solves #7313

Changes

Make TransactionForRpc an abstract base class
Introduce multiple subclasses of TransactionForRpc for specific Transaction types (Legacy, AccessList, EIP1559, Blob and Optimism)
Support JSON (de)serializing
Removed TransactionForRpc -> Transaction "default" conversions: conversions now use domain specific defaults (ex. GasLimit defaults to 90_000) instead of C# specific.
Introduced mechanism to register new subclasses.

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

Added schema verification tests based on the Ethereum JSON-RPC spec (see https://ethereum.github.io/execution-apis/api-documentation/)

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

This PR is an alternative version of #7446 that does not introduce a RpcGenericTransaction for deserializing purposes; instead we reuse the same type hierarchy for both serialization and deserialization. As a quick summary, this PR introduces the following changes:

When deserializing

We read JSON and construct a specific subclass of TransactionForRpc based on the Type field of the incoming JSON object (if this field is missing we default to Legacy transactions).
Each subclass should implement a ToTransaction method that constructs a proper Transaction object while setting up the appropriate domain defaults when fields are missing (ex. if the transaction is AccessList, then the resulting AccessList field in Transaction is an empty access list, not null).

When serializing

We have multiple Converters that know how to take a Transaction and build a TransactionForRpc (the above mentioned abstract base class). Each converter knows how to take a Transaction with a specific TxType and build a subclass (ex. there is a converter from Transaction to BlobTransactionForRpc). Each converter is based on the Ethereum JSON-RPC spec to ensure no fields are missing.
To write JSON, we instruct the serializer to use the runtime type rather than the static type, that is, don't use TransactionForRpc but, for example, EIP1559TransactionForRpc.

- Expose test values and `ValidateSchema`

- Forgot to change the name

- Follows https://github.com/ethereum-optimism/op-geth fields and tests

- Replaces old `AccessListItemForRpc` with proper container

- A lot of questions/unknowns/design decisions

- Mimics `JsonConverter<T>` from `System.Text.Json`

emlautarom1 · 2024-09-30T21:07:59Z

src/Nethermind/Nethermind.Optimism/Rpc/OptimismTransactionForRpc.cs

+        tx.SourceHash = SourceHash ?? throw new ArgumentNullException(nameof(SourceHash));
+        tx.SenderAddress = From ?? throw new ArgumentNullException(nameof(From));
+        tx.To = To;
+        tx.Mint = Mint ?? 0;
+        tx.Value = Value ?? throw new ArgumentNullException(nameof(Value));
+        // TODO: Unsafe cast
+        tx.GasLimit = (long)(Gas ?? throw new ArgumentNullException(nameof(Gas)));
+        tx.IsOPSystemTransaction = IsSystemTx ?? false;
+        tx.Data = Input ?? throw new ArgumentNullException(nameof(Input));


According to the official OP repo these fields are required and we should reject transactions that don't provide it:

https://github.com/ethereum-optimism/op-geth/blob/c283254e5447f127e3b9350860985911dab9cd2f/core/types/transaction_marshalling_test.go#L27-L67

Deposit transaction should never come from RPC? Maybe we should just fail here.

What if we get an invalid (that is, missing fields) deposit transaction on eth_sendTransaction? With these checks we'll fail with a null argument exception. Do you have something different in mind?

LukaszRozmej

Close but few things came up.

src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/AccessListForRpc.cs

LukaszRozmej · 2024-10-01T10:05:34Z

src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs

+            // TODO: For some reason we might get an object wrapped in a String
+            using JsonDocument jsonObject = document.RootElement.ValueKind == JsonValueKind.String
+                ? JsonDocument.Parse(document.RootElement.GetString()!)
+                : document;


Yeah not the biggest fan of that, can we try using type discriminators: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/polymorphism?pivots=dotnet-8-0#customize-the-type-discriminator-name

[JsonPolymorphic(TypeDiscriminatorPropertyName = "TxType")] [JsonDerivedType(typeof(LegacyTransactionForRpc), typeDiscriminator: "Legacy")]

or [JsonDerivedType(typeof(LegacyTransactionForRpc), typeDiscriminator: 0)]

as we would have to make it pluginable maybe we should create custom resolver: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/polymorphism?pivots=dotnet-8-0#configure-polymorphism-with-the-contract-model which would register all the tx types.

LukaszRozmej · 2024-10-01T10:10:07Z

src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs

+        // TODO: We might want to lift `Nonce` to `TransactionForRpc`
+        UInt256? nonce = null;
+        if (rpcTx is LegacyTransactionForRpc legacyTx)
+        {
+            nonce = legacyTx.Nonce;
+        }


Will deposit transaction interact with JSON RPC? Shouldn't it nonce be expected to be outputted there?

Otherwise: true, but Nonce sounds like such a basic transaction functionality that maybe it is better to make an exception, put it in base class and just ignore it or DepositTransaction? Or this shouldn't take in TransactionForRpc but LegacyTransactionForRpc? Or maybe create a class in between called like UserTransactionForRpc?

LukaszRozmej · 2024-10-01T10:12:43Z

src/Nethermind/Nethermind.Optimism/Rpc/OptimismTransactionForRpc.cs

+        tx.SourceHash = SourceHash ?? throw new ArgumentNullException(nameof(SourceHash));
+        tx.SenderAddress = From ?? throw new ArgumentNullException(nameof(From));
+        tx.To = To;
+        tx.Mint = Mint ?? 0;
+        tx.Value = Value ?? throw new ArgumentNullException(nameof(Value));
+        // TODO: Unsafe cast
+        tx.GasLimit = (long)(Gas ?? throw new ArgumentNullException(nameof(Gas)));
+        tx.IsOPSystemTransaction = IsSystemTx ?? false;
+        tx.Data = Input ?? throw new ArgumentNullException(nameof(Input));


Deposit transaction should never come from RPC? Maybe we should just fail here.

- Use `IEnumerable` instead of `List`

emlautarom1 · 2024-10-01T14:00:39Z

Will deposit transaction interact with JSON RPC? Shouldn't it nonce be expected to be outputted there?

Otherwise: true, but Nonce sounds like such a basic transaction functionality that maybe it is better to make an exception, put it in base class and just ignore it or DepositTransaction? Or this shouldn't take in TransactionForRpc but LegacyTransactionForRpc? Or maybe create a class in between called like UserTransactionForRpc?

@LukaszRozmej Nonce is not expected to come as part of deposit transactions and we'll get them in, for example, eth_sendTransaction. The Nonce field is only used during serialization for compatibility reasons and it uses the transaction receipt (see https://specs.optimism.io/protocol/deposits.html?search=#the-deposited-transaction-type). In all other scenarios is treated as 0.

I've decided to change the input of eth_sendTransaction in the OptimismRpcModule module to TransactionForRpc to ensure we're overriding the base EthRpcModule. This override ignores the Nonce field (we let it default to 0 in the base Transaction) without changing the base Eth module behavior.

src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs

- Improve inline docs

…ub.com/NethermindEth/nethermind into refactor/transaction-for-rpc-hierarchy

src/Nethermind/Nethermind.JsonRpc.Test/RpcTest.cs

LukaszRozmej · 2024-10-03T14:34:27Z

src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs

@@ -280,8 +281,12 @@ public ResultWrapper<byte[]> eth_sign(Address addressData, byte[] message)

    public virtual Task<ResultWrapper<Hash256>> eth_sendTransaction(TransactionForRpc rpcTx)
    {
-        Transaction tx = rpcTx.ToTransactionWithDefaults(_blockchainBridge.GetChainId());
-        TxHandlingOptions options = rpcTx.Nonce is null ? TxHandlingOptions.ManagedNonce : TxHandlingOptions.None;
+        Transaction tx = rpcTx.ToTransaction();


Maybe the parameter here should be LegacyTransactionForRpc?

If we do that then we cannot override in OptimismEthRpcModule. We could not override and make the Optimism module use OptimismTransactionForRpc but I'm not sure what method is going to be picked up. Need to double check

emlautarom1 added 30 commits September 4, 2024 16:04

Initial RpcLegacyTransaction

84178b8

Change field order

a2739f4

Support null as ChainId through TransactionBuilder

af3b042

Initial RpcAccessListTransaction

ec1a21e

Add RandomSignatures

88dd0e3

Use TestItem random Signatures

e9994ea

Initial RpcEIP1559Transaction

1839a0d

Rename TestCaseSource

2833a9e

Fix DefaultChainId

3caed48

Guarantee Blob transaction invariants

67ec222

Initial RpcBlobTransaction

0f997c9

Initial IRpcTransaction

5ae394a

Initial IRpcTransactionConverter

14d3cae

Move RpcXTransaction to class hierarchy

11c237e

Merge RPCXTransactionTests into a single test

7580bc4

- Expose test values and `ValidateSchema`

Rename test class

b073e50

- Forgot to change the name

Initial RpcOptimismTransaction

a13fcc0

- Follows https://github.com/ethereum-optimism/op-geth fields and tests

Add WithSourceHash to builder

48326d5

Initial RpcAccessList

16765be

- Replaces old `AccessListItemForRpc` with proper container

Annotate converter instead of registering

f673490

Use annotation for IRpcTransaction

cdd4f23

Initial RpcGenericTransaction

a9f6277

- A lot of questions/unknowns/design decisions

Initial JSON deserialization code

7db803b

Remove TxReceipt for now

0fee0cf

Test for member equality in JSON (de)serialization

19686a1

Introduce ITransactionConverter<T>

38f2a1f

- Mimics `JsonConverter<T>` from `System.Text.Json`

Add TODO

4e040bf

Update inline docs

07690be

Whitespace

92394c1

Delete unused classes

b7c42cf

emlautarom1 and others added 2 commits September 27, 2024 12:57

Remove DefaultChainId hack

1cb3e4f

Merge branch 'master' into refactor/transaction-for-rpc-hierarchy

ac903ad

Nikki601 approved these changes Sep 29, 2024

View reviewed changes

Nikki601 approved these changes Sep 30, 2024

View reviewed changes

emlautarom1 added 3 commits September 30, 2024 12:09

Remove defaulting mechanism for OP transactions

750ad0d

Reject malformed OP transactions

f004f26

Initialize serializer once

ebb50d2

emlautarom1 mentioned this pull request Sep 30, 2024

Refactor TransactionForRpc #7446

Closed

16 tasks

emlautarom1 commented Sep 30, 2024

View reviewed changes

emlautarom1 added 2 commits September 30, 2024 18:14

Merge branch 'master' into refactor/transaction-for-rpc-hierarchy

7738c0f

Formatting

7562257

emlautarom1 marked this pull request as ready for review September 30, 2024 21:19

LukaszRozmej requested changes Oct 1, 2024

View reviewed changes

Reintroduce laziness to AccessListForRpc

0690958

- Use `IEnumerable` instead of `List`

emlautarom1 and others added 3 commits October 1, 2024 11:01

Wrap up Nonce behavior in OP

b785352

Fix Blob transaction code

7c4a9ed

Json Type parsing (#7538)

1c68869

LukaszRozmej reviewed Oct 2, 2024

View reviewed changes

src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs Outdated Show resolved Hide resolved

src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs Outdated Show resolved Hide resolved

emlautarom1 and others added 4 commits October 2, 2024 11:53

Change private TransactionType to struct

58396a8

- Improve inline docs

Merge branch 'master' into refactor/transaction-for-rpc-hierarchy

0ff03ac

Merge branch 'refactor/transaction-for-rpc-hierarchy' of https://gith…

281f6af

…ub.com/NethermindEth/nethermind into refactor/transaction-for-rpc-hierarchy

Avoid double serialization in tests

42c1b29

emlautarom1 commented Oct 3, 2024

View reviewed changes

src/Nethermind/Nethermind.JsonRpc.Test/RpcTest.cs Outdated Show resolved Hide resolved

LukaszRozmej reviewed Oct 3, 2024

View reviewed changes

src/Nethermind/Nethermind.JsonRpc.Test/RpcTest.cs Outdated Show resolved Hide resolved

LukaszRozmej reviewed Oct 3, 2024

View reviewed changes

emlautarom1 mentioned this pull request Oct 3, 2024

Avoid double serialization in RPC tests #7547

Merged

16 tasks

LukaszRozmej approved these changes Oct 3, 2024

View reviewed changes

emlautarom1 added 2 commits October 4, 2024 12:22

Avoid double serialization in RPC tests (#7547)

5002733

Merge branch 'master' into refactor/transaction-for-rpc-hierarchy

686269f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `TransactionForRpc` (Alternative) #7483

Refactor `TransactionForRpc` (Alternative) #7483

emlautarom1 commented Sep 23, 2024 •

edited

Loading

emlautarom1 Sep 30, 2024

LukaszRozmej Oct 1, 2024

emlautarom1 Oct 1, 2024

LukaszRozmej left a comment

LukaszRozmej Oct 1, 2024

LukaszRozmej Oct 1, 2024

LukaszRozmej Oct 1, 2024

emlautarom1 commented Oct 1, 2024

LukaszRozmej Oct 3, 2024

emlautarom1 Oct 3, 2024

Refactor TransactionForRpc (Alternative) #7483

Are you sure you want to change the base?

Refactor TransactionForRpc (Alternative) #7483

Conversation

emlautarom1 commented Sep 23, 2024 • edited Loading

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Remarks

When deserializing

When serializing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszRozmej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `TransactionForRpc` (Alternative) #7483

Refactor `TransactionForRpc` (Alternative) #7483

emlautarom1 commented Sep 23, 2024 •

edited

Loading