Safely cast Optimism subtypes#8516
Conversation
- TODO: Do we need this module now?
- Solved by extending the derived class - Updated tests
| receipt, | ||
| tx.GetGasInfo(spec, block.Header), | ||
| receipts.GetBlockLogFirstIndex(receipt.Index)))]; | ||
| return ResultWrapper<ReceiptForRpc[]?>.Success(result); |
There was a problem hiding this comment.
There is a warning regarding covariant conversions but it should be fine considering that the resulting array is intended to be read-only.
There was a problem hiding this comment.
Yeah, the usual case for it. Having something like IEnumerable<> would be better for the result type (that would address the warning) but that would change a lot of the API surface.
Scooletz
left a comment
There was a problem hiding this comment.
Looks good. One minor question
| receipt, | ||
| tx.GetGasInfo(spec, block.Header), | ||
| receipts.GetBlockLogFirstIndex(receipt.Index)))]; | ||
| return ResultWrapper<ReceiptForRpc[]?>.Success(result); |
There was a problem hiding this comment.
Yeah, the usual case for it. Having something like IEnumerable<> would be better for the result type (that would address the warning) but that would change a lot of the API surface.
| if (transactionModel is DepositTransactionForRpc depositTx) | ||
| { | ||
| OptimismTxReceipt? receipt = receipts.FirstOrDefault(r => r.TxHash?.Equals(hash) ?? false); | ||
| OptimismTxReceipt? receipt = (OptimismTxReceipt?)receipts.FirstOrDefault(r => r.TxHash?.Equals(hash) ?? false); |
There was a problem hiding this comment.
Could you provide a comment or spec link why is this always the case?
There was a problem hiding this comment.
According to the yellow paper, the Receipt type is the same as the Transaction type, so a Deposit transaction should be associated with a Deposit receipt.
If you take a look at op-geth's codebase, when working with a Deposit transaction, the associated receipt contains Optimism's specific fields (ex. DepositNonce), which matches our OptimismTxReceipt type.
Unfortunately, a lot of these invariants are not part of the type system and I haven't been able to find them expressed in the specs so I might be wrong about them.
* Fix `eth_getBlockReceipts` - Do not force casts * Fix `eth_getTransactionReceipt` * Disable broken test * Pending changes * Clean `IOptimismEthRpcModule` - TODO: Do we need this module now? * Fix polymorphic array - Solved by extending the derived class - Updated tests
Fixes #8515
Changes
InvalidCastExceptions.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
The PR is initially marked as draft due to an issue with subtypes and serialization.