feat!: remove T: Transport from public APIs #1859
Conversation
DaniPopes
requested review from
onbjerg,
gakonst,
mattsse,
yash-atreya,
klkvr and
zerosnacks
There was a problem hiding this comment.
removing the (useless) transport generic is long overdue.
only have some questions and a smol ask to describe the alloy-provider <-> sol! macro relationship
we also need to migrate https://github.com/alloy-rs/examples should we do this against this branch or later?
this requires a minor release, I'm fine with doing that whenever but would prefer if we had the examples migrated already
| use std::{ | ||
| future::{Future, IntoFuture}, | ||
| marker::PhantomData, | ||
| pin::Pin, | ||
| }; | ||
|
|
||
| // NOTE: The `T` generic here is kept to mitigate breakage with the `sol!` macro. |
There was a problem hiding this comment.
what's the breakage here?
unclear to me what the relationship with the macro is
EDIT: so the macro in alloy/core generates code for alloy/alloy
this makes upgrading a bit weird, but manageable I assume
| alloy_transport::BoxTransport, | ||
| AnvilProvider<RootProvider<alloy_transport::BoxTransport>, alloy_transport::BoxTransport>, | ||
| PhantomData<MyContract::doStuffCall>, |
| use std::marker::PhantomData; | ||
|
|
||
| /// A handle to an Ethereum contract at a specific address. | ||
| /// | ||
| /// A contract is an abstraction of an executable program on Ethereum. Every deployed contract has | ||
| /// an address, which is used to connect to it so that it may receive messages (transactions). | ||
| #[derive(Clone)] | ||
| pub struct ContractInstance<T, P, N = Ethereum> { | ||
| pub struct ContractInstance<P, N = Ethereum> { |
| // Fake traits to mitigate `sol!` macro breaking changes. | ||
| pub trait Provider<T, N: Network>: alloy_provider::Provider<N> {} | ||
| impl<N: Network, P: alloy_provider::Provider<N>> Provider<(), N> for P {} | ||
|
|
||
| // This is done so that the compiler can infer the `T` type to be `()`, which is the only type | ||
| // that implements this fake `Transport` trait. | ||
| pub trait Transport {} | ||
| impl Transport for () {} |
There was a problem hiding this comment.
could we perhaps add some docs to the release checklist that covers alloy-provider <-> sol! and what we need to keep in mind
There was a problem hiding this comment.
the docs is don't do it, if you have to, message me
| pub trait Transport {} | ||
| impl Transport for () {} |
|
|
||
| /// Admin namespace rpc interface that gives access to several non-standard RPC methods. | ||
| #[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] | ||
| #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] | ||
| pub trait AdminApi<N, T>: Send + Sync { | ||
| pub trait AdminApi<N>: Send + Sync { |
There was a problem hiding this comment.
this network generic is also kinda pointless, and only necessary because the send_request function is part of the Provider trait, we could potentially split these off, but not in this pr
| // SAFETY: `self` is `BoxTransport`. This is a no-op. | ||
| let this = std::mem::ManuallyDrop::new(self); | ||
| return unsafe { std::mem::transmute_copy(&this) }; |
There was a problem hiding this comment.
What's the difference between this and just a regular transmute?
There was a problem hiding this comment.
transmute doesn't allow size mismatch, T has generic size so it never works
There was a problem hiding this comment.
I see, so we effectively just copy the pointer here? hence the manual drop
There was a problem hiding this comment.
yeah, would be just return self
Currently, every high-level type derived from RPC client contains a
T: Transport + Clone = BoxTransportgeneric parameter.However, there is, AFAICT, no benefit to exposing this generic in high-level types. The dynamic dispatch performance impact is negligible compared to what a transport actually does (many syscalls, network calls, etc.), and it only complicates generic bounds where any of those types are used.
T: Transport (+ Clone)generic parameters from almost every public API by always boxing the transport internally.IntoBoxTransportfor where this generic is still needed, mainly the builder/layer types.BoxTransportConnectand rename (with deprecation)connect_boxedmethods to simpler names.connect_builtinand similar to justconnect.boxed()methods, as they just returnselfnowBreaking changes have been adjusted to not require any changes in alloy-core (
sol!macro); mainly,CallBuilderandEventwill keep theTgeneric, but the trait bounds are replaced with a fakeTransporttrait only exposed tosol!, so that the compiler is able to infer the type to be(), the only implementer of that fake trait.