use result for `TransactionCompact::fill`. #12170

lakshya-sky · 2024-10-29T16:11:12Z

Add an associated type for TransactionCompact and use it for fill return type.

Closes #12020

emhane

looking good

crates/optimism/rpc/src/eth/transaction.rs

crates/rpc/rpc-types-compat/src/transaction/mod.rs

crates/optimism/rpc/src/eth/transaction.rs

crates/rpc/rpc-types-compat/src/transaction/mod.rs

lakshya-sky · 2024-10-30T12:55:15Z

hey @emhane, I tried to fix the suggestions but a lot places needs error conversions, which might require a little context, so I've used expect at those places. If you could let me know which error variant is best? Thank you!

emhane

nice progress!

crates/rpc/rpc-eth-api/src/core.rs

* Add an associated type for `TransactionCompact` and use it for `fill` return type.

lakshya-sky · 2024-11-05T23:03:29Z

@emhane your previous commit helped me understand the issue correctly. You're a Rockstar! 🌟

emhane

still a bit to go!

emhane · 2024-11-06T13:41:05Z

crates/optimism/rpc/src/eth/transaction.rs

        let signed_tx = tx.clone().into_signed();
        let hash = tx.hash;

-        let mut inner = EthTxBuilder.fill(tx, tx_info);
+        let mut inner =
+            EthTxBuilder.fill(tx, tx_info).expect("EthTxBuilder.fill should be infallible");


only this call to fill can be expected to be infallible. all the other expect statements need to be removed.

Rjected

just one comment

Rjected · 2024-11-06T17:29:30Z

crates/rpc/rpc/src/eth/helpers/types.rs

@@ -20,11 +21,13 @@ where
 {
    type Transaction = <Ethereum as Network>::TransactionResponse;

+    type TransactionError = EthApiError;


this can be the Infallible type right?

it can, and was initially set to infallible by @lakshya-sky . however it became complicated converting the error to an EthApiError and OpEthApiError in order to propagate it, it required explicit trait bounds for the conversion all over the place and got very messy

hmmm, I see, I'm assuming it wasn't as easy as unwrapping when this was called at a higher level?

emhane · 2024-11-06T19:44:17Z

crates/rpc/rpc-eth-api/src/core.rs

+            .map(|tx| tx.into_transaction(self.tx_resp_builder()))
+            .transpose()
+            .map_err(|err| internal_rpc_err(format!("failed to convert transaction: {}", err)))?)


Suggested change

.map(|tx| tx.into_transaction(self.tx_resp_builder()))

.transpose()

.map_err(|err| internal_rpc_err(format!("failed to convert transaction: {}", err)))?)

.map(|tx| tx.into_transaction(self.tx_resp_builder()))

.transpose()?

doesn't this work?

I couldn't get this to work. no matter what I try its not able to convert the error types.

I'll try again later to see if I can fix that.

hm ok, perhaps you must explicitly add the trait bound on the trait impl in that case, smthg like

impl<T> EthApiServer< RpcTransaction<T::NetworkTypes>, RpcBlock<T::NetworkTypes>, RpcReceipt<T::NetworkTypes>, > for T where T: FullEthApi, jsonrpsee_types::error::ErrorObject<'static>: From<T::Error>, jsonrpsee_types::error::ErrorObject<'static>: From<<T::TransactionCompat as TransactionCompat>::TransactionError>,

emhane

nice progress, getting close to done

crates/optimism/rpc/src/eth/transaction.rs

emhane · 2024-11-07T09:37:19Z

crates/rpc/rpc/src/eth/filter.rs

-            ))
+            pending_txs.push(
+                from_recovered(tx.transaction.to_recovered_transaction(), &self.tx_resp_builder)
+                    .expect("fill should be infallible"),


this expect still needs to go

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).

emhane · 2024-11-07T09:37:37Z

crates/rpc/rpc/src/eth/pubsub.rs

+                                    tx.transaction.to_recovered_transaction(),
+                                    &tx_resp_builder,
+                                )
+                                .expect("fill should be infallible"),


this expect still needs to go

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.

crates/rpc/rpc/src/txpool.rs

crates/rpc/rpc-types-compat/src/transaction/mod.rs

emhane · 2024-11-07T09:39:20Z

crates/rpc/rpc-types-compat/src/block.rs

@@ -88,7 +88,8 @@ pub fn from_block_full<T: TransactionCompat>(

            from_recovered_with_block_context::<T>(signed_tx_ec_recovered, tx_info, tx_resp_builder)
        })
-        .collect::<Vec<_>>();
+        .collect::<Result<Vec<_>, T::TransactionError>>()
+        .expect("fill should be infallible");


this expect still needs to go

For this one what do you think is the best idea? I was thinking to add TransactionCompatError inside alloy_rpc_eth::BlockError and maping the error to BlockError. Which would require making a pr to alloy.

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>) or BlockError::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.

actually makes most sense to change the signature of from_block_full to return TransactionCompat::Error, and add trait bound From<BlockError> to the associated type

crates/optimism/rpc/src/eth/transaction.rs

crates/optimism/rpc/src/error.rs

emhane

great job!

emhane · 2024-11-08T06:05:32Z

crates/rpc/rpc-types-compat/src/block.rs

@@ -88,7 +88,8 @@ pub fn from_block_full<T: TransactionCompat>(

            from_recovered_with_block_context::<T>(signed_tx_ec_recovered, tx_info, tx_resp_builder)
        })
-        .collect::<Vec<_>>();
+        .collect::<Result<Vec<_>, T::TransactionError>>()
+        .expect("fill should be infallible");


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>) or BlockError::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.

emhane · 2024-11-08T06:07:03Z

crates/rpc/rpc/src/eth/filter.rs

+            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");


Suggested change

error!(target: "rpc", %err, "Failed to fill txn with block context");

error!(target: "rpc::filter", %err, "Failed to fill txn with block context");

nitpick, makes the target more helpful

nonetheless, this error should be propagated, but can be handled separately, could you please open an issue

emhane · 2024-11-08T07:01:23Z

crates/rpc/rpc/src/eth/pubsub.rs

+                        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)


same here, this error should be propagated, or, sent as a message i.e. add new variant SubscriptionMessageInner::Error(Box<dyn Error>). please open an issue.

addressed

lakshya-sky · 2024-11-12T17:17:32Z

@emhane you fixed this succinctly! Cheers!

lakshya-sky requested review from mattsse, Rjected, emhane and fgimenez as code owners October 29, 2024 16:11

emhane requested changes Oct 29, 2024

View reviewed changes

crates/optimism/rpc/src/eth/transaction.rs Outdated Show resolved Hide resolved

crates/rpc/rpc-types-compat/src/transaction/mod.rs Outdated Show resolved Hide resolved

lakshya-sky requested a review from emhane October 29, 2024 17:57

emhane requested changes Oct 30, 2024

View reviewed changes

emhane reviewed Nov 3, 2024

View reviewed changes

crates/rpc/rpc-eth-api/src/core.rs Outdated Show resolved Hide resolved

lakshya-sky added 3 commits November 4, 2024 11:09

use result for TransactionCompact::fill.

0d77957

* Add an associated type for `TransactionCompact` and use it for `fill` return type.

fix suggestions

279fe00

use expect for erorrs

52e731d

lakshya-sky force-pushed the result-for-transaction-compact-fill branch from c90a97b to 52e731d Compare November 4, 2024 16:24

emhane and others added 2 commits November 5, 2024 10:45

Use EthApiTypes::Error for TransactionCompat error

64abab3

fix type errors

95a304f

emhane requested changes Nov 6, 2024

View reviewed changes

Rjected previously requested changes Nov 6, 2024

View reviewed changes

change expect to error

63dfd2d

emhane reviewed Nov 6, 2024

View reviewed changes

lakshya-sky added 2 commits November 6, 2024 16:01

removed expects and used error propagation

c55411f

merge main

7cf5d05

emhane requested changes Nov 7, 2024

View reviewed changes

lakshya-sky added 2 commits November 8, 2024 00:40

remove expects and remove dead code

9be928f

merge main

309a7a6

emhane reviewed Nov 8, 2024

View reviewed changes

mattsse force-pushed the main branch from b8bc74a to 32ebb18 Compare November 8, 2024 13:31

This was referenced Nov 11, 2024

Progagate TrasactionCompat::Error instead of logging for rpc pubsub. #12456

Open

Progagate TrasactionCompat::Error instead of logging for rpc filter. #12457

Open

propagate transaction compat error for block building rpcs

6cde301

lakshya-sky requested a review from gakonst as a code owner November 11, 2024 19:19

lakshya-sky and others added 4 commits November 11, 2024 14:34

merge main

523c03e

Remove redundant trait bounds on TransactionCompat::Error

181528c

Remove expect from tx pool api

b59f404

Fix lint

f0c0a41

emhane approved these changes Nov 12, 2024

View reviewed changes

emhane enabled auto-merge November 12, 2024 10:18

emhane added this pull request to the merge queue Nov 12, 2024

Merged via the queue into paradigmxyz:main with commit bad7a4f Nov 12, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use result for `TransactionCompact::fill`. #12170

use result for `TransactionCompact::fill`. #12170

lakshya-sky commented Oct 29, 2024

emhane left a comment

lakshya-sky commented Oct 30, 2024

emhane left a comment

lakshya-sky commented Nov 5, 2024 •

edited

Loading

emhane left a comment

emhane Nov 6, 2024

Rjected left a comment

Rjected Nov 6, 2024

emhane Nov 6, 2024

Rjected Nov 6, 2024

emhane Nov 6, 2024

lakshya-sky Nov 6, 2024

lakshya-sky Nov 6, 2024

emhane Nov 6, 2024

emhane left a comment

emhane Nov 7, 2024

lakshya-sky Nov 8, 2024

emhane Nov 7, 2024

lakshya-sky Nov 8, 2024

emhane Nov 7, 2024

lakshya-sky Nov 8, 2024

emhane Nov 8, 2024

emhane Nov 10, 2024 •

edited

Loading

emhane left a comment

emhane Nov 8, 2024

emhane Nov 8, 2024

emhane Nov 8, 2024

lakshya-sky commented Nov 12, 2024

	error!(target: "rpc", %err, "Failed to fill txn with block context");
	error!(target: "rpc::filter", %err, "Failed to fill txn with block context");

use result for TransactionCompact::fill. #12170

use result for TransactionCompact::fill. #12170

Conversation

lakshya-sky commented Oct 29, 2024

emhane left a comment

Choose a reason for hiding this comment

lakshya-sky commented Oct 30, 2024

emhane left a comment

Choose a reason for hiding this comment

lakshya-sky commented Nov 5, 2024 • edited Loading

emhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

emhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya-sky commented Nov 12, 2024

use result for `TransactionCompact::fill`. #12170

use result for `TransactionCompact::fill`. #12170

lakshya-sky commented Nov 5, 2024 •

edited

Loading

emhane Nov 10, 2024 •

edited

Loading