-
Notifications
You must be signed in to change notification settings - Fork 430
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
(Alternative)
#7483
base: master
Are you sure you want to change the base?
Conversation
- 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`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close but few things came up.
src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/AccessListForRpc.cs
Outdated
Show resolved
Hide resolved
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// TODO: We might want to lift `Nonce` to `TransactionForRpc` | ||
UInt256? nonce = null; | ||
if (rpcTx is LegacyTransactionForRpc legacyTx) | ||
{ | ||
nonce = legacyTx.Nonce; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deposit transaction should never come from RPC? Maybe we should just fail here.
- Use `IEnumerable` instead of `List`
@LukaszRozmej I've decided to change the input of |
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
- Improve inline docs
…ub.com/NethermindEth/nethermind into refactor/transaction-for-rpc-hierarchy
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the parameter here should be LegacyTransactionForRpc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- Add initial spec tests
- Replaces `AuthorizationTupleForRpc` - Several TODOs
// TODO: Are these fields actually nullable | ||
public UInt256? YParity { get; set; } | ||
public UInt256? S { get; set; } | ||
public UInt256? R { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ak88 are these fields actually nullable or can we just default them to 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they cannot be null
tuple.Address, | ||
tuple.Nonce, | ||
// TODO: This might be wrong | ||
new Signature(tuple.R ?? 0, tuple.S ?? 0, (ulong)(tuple.YParity ?? 0))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can drop the nullability, and some of the types might be changed for devnet-4, but should be fine for now
Partially solves #7313
Changes
TransactionForRpc
an abstract base classTransactionForRpc
for specific Transaction types (Legacy
,AccessList
,EIP1559
,Blob
andOptimism
)TransactionForRpc -> Transaction
"default" conversions: conversions now use domain specific defaults (ex.GasLimit
defaults to90_000
) instead of C# specific.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
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
TransactionForRpc
based on theType
field of the incoming JSON object (if this field is missing we default toLegacy
transactions).ToTransaction
method that constructs a properTransaction
object while setting up the appropriate domain defaults when fields are missing (ex. if the transaction isAccessList
, then the resultingAccessList
field inTransaction
is an empty access list, notnull
).When serializing
Converter
s that know how to take aTransaction
and build aTransactionForRpc
(the above mentioned abstract base class). Each converter knows how to take a Transaction with a specificTxType
and build a subclass (ex. there is a converter fromTransaction
toBlobTransactionForRpc
). Each converter is based on the Ethereum JSON-RPC spec to ensure no fields are missing.TransactionForRpc
but, for example,EIP1559TransactionForRpc
.