-
Notifications
You must be signed in to change notification settings - Fork 477
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
Proper Transaction type
for eth_*
calls when running Optimism
#8157
base: master
Are you sure you want to change the base?
Conversation
- Rename `OptimismTransactionForRpc` to `DepositTransactionForRpc` - Do not map everything to `DepositTransactionForRpc` - Remove `new` methods on `IOptimismEthRpcModule`
- Remove unused `using`s
- Only for `Deposit` transactions - Remove intermediate `ToArray`
{ | ||
OptimismTxReceipt? receipt = _receiptFinder | ||
.Get(block) | ||
.Cast<OptimismTxReceipt>() |
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.
Are all receipts always of type OptimismTxReceipt
or is it possible to have a TxReceipt
? If the later is correct then this cast will fail at runtime.
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.
Potentially you could make EthRpcModule
bit more extensible and avoid some code duplication.
- Remove duplication - Rollback virtual modifiers - Log on `Success` and `Trace` only
- Same behavior as base class
- Same behavior as base class
…pe' into fix/optimism-eth-rpc-module-txtype
I've removed the duplication in the |
Changes
type
instead of always using0x7E
("deposit")Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added unit tests to ensure that we're not changing
TxType
and we're including additional receipt information for "deposit" transactions only.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
I'm curious as to why we didn't get this reported earlier considering that the issue has been present since #7483.