-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
.
#12170
Changes from 1 commit
0d77957
279fe00
52e731d
64abab3
95a304f
63dfd2d
c55411f
7cf5d05
9be928f
309a7a6
6cde301
523c03e
181528c
b59f404
f0c0a41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -34,7 +34,7 @@ use tokio::{ | |||||
sync::{mpsc::Receiver, Mutex}, | ||||||
time::MissedTickBehavior, | ||||||
}; | ||||||
use tracing::trace; | ||||||
use tracing::{error, trace}; | ||||||
|
||||||
/// The maximum number of headers we read at once when handling a range filter. | ||||||
const MAX_HEADERS_RANGE: u64 = 1_000; // with ~530bytes per header this is ~500kb | ||||||
|
@@ -625,10 +625,12 @@ where | |||||
let mut prepared_stream = self.txs_stream.lock().await; | ||||||
|
||||||
while let Ok(tx) = prepared_stream.try_recv() { | ||||||
pending_txs.push( | ||||||
from_recovered(tx.transaction.to_recovered_transaction(), &self.tx_resp_builder) | ||||||
.expect("fill should be infallible"), | ||||||
); | ||||||
match from_recovered(tx.transaction.to_recovered_transaction(), &self.tx_resp_builder) { | ||||||
Ok(tx) => pending_txs.push(tx), | ||||||
Err(err) => { | ||||||
error!(target: "rpc", %err, "Failed to fill txn with block context"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
nitpick, makes the target more helpful nonetheless, this error should be propagated, but can be handled separately, could you please open an issue |
||||||
} | ||||||
} | ||||||
} | ||||||
FilterChanges::Transactions(pending_txs) | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,14 +146,20 @@ where | |
match params { | ||
Params::Bool(true) => { | ||
// full transaction objects requested | ||
let stream = pubsub.full_pending_transaction_stream().map(|tx| { | ||
EthSubscriptionResult::FullTransaction(Box::new( | ||
from_recovered( | ||
tx.transaction.to_recovered_transaction(), | ||
&tx_resp_builder, | ||
) | ||
.expect("fill should be infallible"), | ||
)) | ||
let stream = pubsub | ||
.full_pending_transaction_stream() | ||
.filter_map(|tx| { | ||
let tx_value = match from_recovered( | ||
tx.transaction.to_recovered_transaction(), | ||
&tx_resp_builder, | ||
){ | ||
Ok(tx) => Some(EthSubscriptionResult::FullTransaction(Box::new(tx))), | ||
Err(err) => { | ||
tracing::error!(target = "rpc", %err, "Failed to fill transaction with block context"); | ||
None | ||
} | ||
}; | ||
std::future::ready(tx_value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, this error should be propagated, or, sent as a message i.e. add new variant |
||
}); | ||
return pipe_from_stream(accepted_sink, stream).await | ||
} | ||
|
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 type