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.
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
use result for
TransactionCompact::fill
. #12170use result for
TransactionCompact::fill
. #12170Changes from 8 commits
0d77957
279fe00
52e731d
64abab3
95a304f
63dfd2d
c55411f
7cf5d05
9be928f
309a7a6
6cde301
523c03e
181528c
b59f404
f0c0a41
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this expect still needs to go
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.
For this one what do you think is the best idea? I was thinking to add
TransactionCompatError
insidealloy_rpc_eth::BlockError
and maping the error toBlockError
. Which would require making a pr to alloy.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.
yes indeed it does require a pr to alloy, you're right. suggest making a pr to alloy adding error variant
BlockError::Custom(Box<dyn Error>)
orBlockError::Custom(String)
- would you please open the pr?not to block this pr until next alloy release, let's log the error here, and would you please open an issue to propagate the error here instead when the alloy change is available.
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.
actually makes most sense to change the signature of
from_block_full
to returnTransactionCompat::Error
, and add trait boundFrom<BlockError>
to the associated typeThere 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.
this expect still needs to go
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.
I've removed the expect and tried logging the error so that it doesn't become nightmare to propagate the error.(I tried propagating it first but requires changing a lot of types).
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.
this can be the
Infallible
type right?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.
it can, and was initially set to infallible by @lakshya-sky . however it became complicated converting the error to an
EthApiError
andOpEthApiError
in order to propagate it, it required explicit trait bounds for the conversion all over the place and got very messyThere 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.
hmmm, I see, I'm assuming it wasn't as easy as unwrapping when this was called at a higher level?
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.
this expect still needs to go
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.
Here also I've tried to log the error, otherwise it requires
TransactionCompat::Error : Serialize + Send
, which is something I don't know how to do.