feat: integrate EvmOverrides to rpc types
#906
Conversation
tcoratger
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg,
prestwich and
Evalir
as code owners
| } | ||
|
|
||
| /// Creates a new instance with the given state overrides. | ||
| pub const fn state(state: Option<StateOverride>) -> Self { |
There was a problem hiding this comment.
why even allow None as an input?
There was a problem hiding this comment.
this is also mainly useful from server POV because some methods only support StateOverrides or BlockOverrides and usually as Option.
There was a problem hiding this comment.
My main point is that passing None to this function is always more clearly represented as Default::default which means this API is an artifact of a specific usage, not a good API on its own, and is not something we should make available to non-reth users
// `fn state(state:StateOverride) -> Self`
state_opt.map(EvmOverrides::state).unwrap_or_default()
// current api
EvmOverrides::state(state_opt)
// From<Option<_>> feels cleanest
state_opt.into()
There was a problem hiding this comment.
/// Matches some other spots in reth
EvmOverrides::default().maybe_state(state_opt)
| } | ||
|
|
||
| /// Creates a new instance with the given block overrides. | ||
| pub const fn block(block: Option<Box<BlockOverrides>>) -> Self { |
crates/rpc-types-eth/src/state.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl From<Option<StateOverride>> for EvmOverrides { |
There was a problem hiding this comment.
impl From<StateOverrides>
impl From<Box<BlockOverrides>>
impl From<Option<Box<BlockOverrides>>>
There was a problem hiding this comment.
I'd actually like to remove this from impl entirely, the caller should call EvmOverrides::state instead
| pub state: Option<StateOverride>, | ||
| /// Applies overrides to the block before execution. | ||
| /// | ||
| /// This is a `Box` because less common and only available in debug trace endpoints. |
There was a problem hiding this comment.
if this type's only purpose is to be serialized, it could instead have borrows of each component?
There was a problem hiding this comment.
these are only really useful from the server POV and are intended to bundle overrides that are separate arguments
There was a problem hiding this comment.
so does this get deserialized as a unit? or does it get deser separately and then constructed?
There was a problem hiding this comment.
separately then constructed, so in the method implementation we can pass overrides: EvmOverrides instead of two Option arguments
There was a problem hiding this comment.
this smells like the API problem is minimizing diff for some existing-but-messy code. don't love it, but i won't block you merging it
There was a problem hiding this comment.
the issue here is that some endpoints support both override args or none at all, some only state, some only block, but a lot require the same setup, like eth_call, trace_call, call_many, estimate.
this helper type makes it possible to reuse the same setup
| pub state: Option<StateOverride>, | ||
| /// Applies overrides to the block before execution. | ||
| /// | ||
| /// This is a `Box` because less common and only available in debug trace endpoints. |
There was a problem hiding this comment.
these are only really useful from the server POV and are intended to bundle overrides that are separate arguments
| } | ||
|
|
||
| /// Creates a new instance with the given state overrides. | ||
| pub const fn state(state: Option<StateOverride>) -> Self { |
There was a problem hiding this comment.
this is also mainly useful from server POV because some methods only support StateOverrides or BlockOverrides and usually as Option.
crates/rpc-types-eth/src/state.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl From<Option<StateOverride>> for EvmOverrides { |
There was a problem hiding this comment.
I'd actually like to remove this from impl entirely, the caller should call EvmOverrides::state instead
@mattsse As a result of these discussions I've just removed the extra |
* feat: integrate EvmOverrides to rpc types * fix clippy * remove From impl
To replace https://github.com/paradigmxyz/reth/blob/c694433675c3fed05fbe72c50476f4a195acafc6/crates/rpc/rpc/src/eth/revm_utils.rs#L30-L41