Refactor TransactionForRpc (Alternative)#7483
Merged
Merged
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`
…ub.com/NethermindEth/nethermind into refactor/transaction-for-rpc-hierarchy
emlautarom1
commented
Oct 3, 2024
LukaszRozmej
reviewed
Oct 3, 2024
LukaszRozmej
reviewed
Oct 3, 2024
| { | ||
| Transaction tx = rpcTx.ToTransactionWithDefaults(_blockchainBridge.GetChainId()); | ||
| TxHandlingOptions options = rpcTx.Nonce is null ? TxHandlingOptions.ManagedNonce : TxHandlingOptions.None; | ||
| Transaction tx = rpcTx.ToTransaction(); |
Member
There was a problem hiding this comment.
Maybe the parameter here should be LegacyTransactionForRpc?
Contributor
Author
There was a problem hiding this comment.
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
16 tasks
LukaszRozmej
approved these changes
Oct 3, 2024
- Add initial spec tests
- Replaces `AuthorizationTupleForRpc` - Several TODOs
Nikki601
approved these changes
Oct 8, 2024
emlautarom1
commented
Oct 8, 2024
emlautarom1
commented
Oct 8, 2024
5 tasks
ak88
approved these changes
Oct 14, 2024
rubo
reviewed
Nov 4, 2024
| while (true) | ||
| { | ||
| ReceiveResult? result = await stream.ReceiveAsync(buffer); | ||
| ReceiveResult? result = await stream.ReceiveAsync(buffer).ConfigureAwait(false); |
16 tasks
This was referenced Feb 20, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially solves #7313
Changes
TransactionForRpcan abstract base classTransactionForRpcfor specific Transaction types (Legacy,AccessList,EIP1559,BlobandOptimism)TransactionForRpc -> Transaction"default" conversions: conversions now use domain specific defaults (ex.GasLimitdefaults 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
RpcGenericTransactionfor 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
TransactionForRpcbased on theTypefield of the incoming JSON object (if this field is missing we default toLegacytransactions).ToTransactionmethod that constructs a properTransactionobject while setting up the appropriate domain defaults when fields are missing (ex. if the transaction isAccessList, then the resultingAccessListfield inTransactionis an empty access list, notnull).When serializing
Converters that know how to take aTransactionand build aTransactionForRpc(the above mentioned abstract base class). Each converter knows how to take a Transaction with a specificTxTypeand build a subclass (ex. there is a converter fromTransactiontoBlobTransactionForRpc). Each converter is based on the Ethereum JSON-RPC spec to ensure no fields are missing.TransactionForRpcbut, for example,EIP1559TransactionForRpc.