Skip to content
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

feat: remove constraint of concrete type AnyTransactionReceipt for EthApiTypes::NetworkType #10693

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

nkysg
Copy link
Contributor

@nkysg nkysg commented Sep 4, 2024

Ref #10632

@nkysg nkysg marked this pull request as draft September 4, 2024 14:59
@mattsse mattsse added the A-rpc Related to the RPC implementation label Sep 4, 2024
@nkysg
Copy link
Contributor Author

nkysg commented Sep 4, 2024

error[E0599]: the method `into_rpc` exists for struct `OtterscanApi<EthApi>`, but its trait bounds were not satisfied
    --> crates/rpc/rpc-builder/src/lib.rs:1122:82
     |
1122 |                         RethRpcModule::Ots => OtterscanApi::new(eth_api.clone()).into_rpc().into(),
     |                                                                                  ^^^^^^^^
     |
    ::: /Users/ysg/Github/reth/crates/rpc/rpc/src/otterscan.rs:33:1
     |
33   | pub struct OtterscanApi<Eth> {
     | ---------------------------- doesn't satisfy 5 bounds
     |
     = note: the following trait bounds were not satisfied:
             `OtterscanApi<EthApi>: EthApiTypes`
             which is required by `OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `OtterscanApi<EthApi>: FullEthApi`
             which is required by `OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `<OtterscanApi<EthApi> as EthApiTypes>::NetworkTypes = _`
             which is required by `OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `<<EthApi as EthApiTypes>::NetworkTypes as Network>::ReceiptResponse = WithOtherFields<TransactionReceipt<AnyReceiptEnvelope<reth_rpc_types::Log>>>`
             which is required by `OtterscanApi<EthApi>: reth_rpc_api::OtterscanServer`
             `&OtterscanApi<EthApi>: EthApiTypes`
             which is required by `&OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `&OtterscanApi<EthApi>: FullEthApi`
             which is required by `&OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `<&OtterscanApi<EthApi> as EthApiTypes>::NetworkTypes = _`
             which is required by `&OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `&mut OtterscanApi<EthApi>: EthApiTypes`
             which is required by `&mut OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `&mut OtterscanApi<EthApi>: FullEthApi`
             which is required by `&mut OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`
             `<&mut OtterscanApi<EthApi> as EthApiTypes>::NetworkTypes = _`
             which is required by `&mut OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `reth-rpc-builder` (lib) due to 1 previous error

need to fix build error

@nkysg nkysg force-pushed the rm_receipt branch 2 times, most recently from 128802c to 78d866e Compare September 5, 2024 16:12
@@ -1100,7 +1119,8 @@ where
)
.into_rpc()
.into(),
RethRpcModule::Ots => OtterscanApi::new(eth_api.clone()).into_rpc().into(),
RethRpcModule::Ots => todo!(), /* OtterscanApi::new(eth_api.clone()). */
// into_rpc().into(),
Copy link
Contributor Author

@nkysg nkysg Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emhane I don't know how to address this. the error info #10693 (comment)

@nkysg
Copy link
Contributor Author

nkysg commented Sep 5, 2024

@emhane, sorry for that, I use force push.I just want to re implement.

@nkysg nkysg marked this pull request as ready for review September 5, 2024 16:19
@nkysg nkysg requested a review from gakonst as a code owner September 5, 2024 16:19
@emhane
Copy link
Member

emhane commented Sep 5, 2024

 pub struct OtterscanApi<Eth> {
     | ---------------------------- doesn't satisfy 5 bounds
     |
     = note: the following trait bounds were not satisfied:
             `reth_rpc::OtterscanApi<EthApi>: reth_rpc_eth_api::EthApiTypes`
             which is required by `reth_rpc::OtterscanApi<EthApi>: reth_rpc_api::EthApiServer<_, reth_rpc_types::Block<_, _>, _>`

this is requiring that OtterscanApi be implemented for EthApiTypes with the concrete receipt type.AnyTransactionReceipt, otherwise EthApiServer isn't implemented for OtterscanApi. this is since the blanket implementation of EthApiServer requires that EthApiServer and EthApiTypes use the same receipt type.

receipt type taken from T which impl FullEthApi

impl<T>
EthApiServer<
RpcTransaction<T::NetworkTypes>,
RpcBlock<T::NetworkTypes>,
RpcReceipt<T::NetworkTypes>,
> for T
where
T: FullEthApi,

EthApiTypes is a super trait of FullEthApi
/// Helper trait to unify all `eth` rpc server building block traits, for simplicity.
///
/// This trait is automatically implemented for any type that implements all the `Eth` traits.
pub trait FullEthApi:
EthApiTypes

@emhane
Copy link
Member

emhane commented Sep 5, 2024

@emhane, sorry for that, I use force push.I just want to re implement.

alright then you saw commit 128802c?

Comment on lines 795 to 807
EthApi: EthApiServer<
reth_rpc_types::Transaction,
reth_rpc_types::Block,
reth_rpc_types::AnyTransactionReceipt,
>,
RpcTransaction<EthApi::NetworkTypes>,
RpcBlock<EthApi::NetworkTypes>,
RpcReceipt<EthApi::NetworkTypes>,
> + EthApiTypes<
NetworkTypes: alloy_network::Network<
TransactionResponse = reth_rpc_types::Transaction,
HeaderResponse = reth_rpc_types::Header,
ReceiptResponse = AnyTransactionReceipt,
>,
>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave concrete types for transaction and block for now everywhere, only relax the constraint on receipt. so we want

 EthApi: EthApiServer<
            reth_rpc_types::Transaction,
            reth_rpc_types::Block,
            RpcReceipt<EthApi::NetworkTypes>,
        >,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I will address this.

@nkysg
Copy link
Contributor Author

nkysg commented Sep 5, 2024

@emhane, sorry for that, I use force push.I just want to re implement.

alright then you saw commit 128802c?

got it.

@nkysg nkysg marked this pull request as draft September 5, 2024 16:28
@nkysg nkysg marked this pull request as ready for review September 6, 2024 15:36
@nkysg nkysg requested a review from onbjerg as a code owner September 6, 2024 15:36
@nkysg nkysg requested a review from emhane September 6, 2024 15:52
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked by alloy-rs/alloy#1245

@@ -369,7 +369,7 @@ impl<T>
RpcReceipt<T::NetworkTypes>,
> for T
where
T: FullEthApi,
T: FullEthApi<NetworkTypes: Network<ReceiptResponse = AnyTransactionReceipt>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the opposite of what we want, we want to remove this constraint. however, the possibility of doing so depends partly on alloy-rs/alloy#1245.

@emhane emhane added the S-blocked This cannot more forward until something else changes label Sep 6, 2024
@nkysg nkysg requested a review from emhane September 6, 2024 23:11
@emhane emhane removed the S-blocked This cannot more forward until something else changes label Sep 8, 2024
@@ -76,6 +79,7 @@ where
> + EthApiTypes<
NetworkTypes: Network<
TransactionResponse = WithOtherFields<reth_rpc_types::Transaction>,
ReceiptResponse = AnyTransactionReceipt,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constraint can now be removed since alloy-rs/alloy#1245 is closed. the new trait methods on alloy_network_primitives::ReceiptResponse should be enough to access fields on the receipt throughout this OtterscanServer impl.

@emhane
Copy link
Member

emhane commented Sep 9, 2024

this is one of prs that is also needed to unblock replacing AnyTransactionReceipt with RpcReceipt<Self::NetworkTypes> in EthApi trait methods #10781 @nkysg

@nkysg
Copy link
Contributor Author

nkysg commented Sep 9, 2024

Thanks. Got it.

@emhane
Copy link
Member

emhane commented Sep 9, 2024

you know what, a better approach here is to change the description to "Ref <issue>" instead of "Closes <issue>", and merge this PR :)

@nkysg
Copy link
Contributor Author

nkysg commented Sep 9, 2024

Thanks for correcting me. I saw many guys use closes, so I think it's right. I have modified it.

@emhane
Copy link
Member

emhane commented Sep 9, 2024

Thanks for correcting me. I saw many guys use closes, so I think it's right. I have modified it.

no worries, close is for when the issue is solved, and ref when it is related e.g. partly solves the issue

@emhane emhane enabled auto-merge September 9, 2024 13:35
@emhane emhane added this pull request to the merge queue Sep 9, 2024
Merged via the queue into paradigmxyz:main with commit b78e0f6 Sep 9, 2024
35 checks passed
@nkysg nkysg deleted the rm_receipt branch September 10, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants