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: integrate EvmOverrides to rpc types #906

Merged
merged 3 commits into from
Jun 16, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions crates/rpc-types-eth/src/state.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! bindings for state overrides in eth_call

use crate::BlockOverrides;
use alloy_primitives::{Address, Bytes, B256, U256, U64};
use serde::{Deserialize, Serialize};
use std::collections::HashMap;
Expand Down Expand Up @@ -30,6 +31,64 @@ pub struct AccountOverride {
pub state_diff: Option<HashMap<B256, B256>>,
}

/// Helper type that bundles various overrides for EVM Execution.
///
/// By `Default`, no overrides are included.
#[derive(Debug, Clone, Default)]
pub struct EvmOverrides {
/// Applies overrides to the state before execution.
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.
Copy link
Member

Choose a reason for hiding this comment

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

if this type's only purpose is to be serialized, it could instead have borrows of each component?

Copy link
Member

Choose a reason for hiding this comment

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

these are only really useful from the server POV and are intended to bundle overrides that are separate arguments

Copy link
Member

Choose a reason for hiding this comment

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

so does this get deserialized as a unit? or does it get deser separately and then constructed?

Copy link
Member

Choose a reason for hiding this comment

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

separately then constructed, so in the method implementation we can pass overrides: EvmOverrides instead of two Option arguments

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@mattsse mattsse Jun 16, 2024

Choose a reason for hiding this comment

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

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 block: Option<Box<BlockOverrides>>,
}

impl EvmOverrides {
/// Creates a new instance with the given overrides
pub const fn new(state: Option<StateOverride>, block: Option<Box<BlockOverrides>>) -> Self {
Self { state, block }
}

/// Creates a new instance with the given state overrides.
pub const fn state(state: Option<StateOverride>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

why even allow None as an input?

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 also mainly useful from server POV because some methods only support StateOverrides or BlockOverrides and usually as Option.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

/// Matches some other spots in reth
EvmOverrides::default().maybe_state(state_opt)

Self { state, block: None }
}

/// Creates a new instance with the given block overrides.
pub const fn block(block: Option<Box<BlockOverrides>>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

same question

Self { state: None, block }
}

/// Returns `true` if the overrides contain state overrides.
pub const fn has_state(&self) -> bool {
self.state.is_some()
}

/// Returns `true` if the overrides contain block overrides.
pub const fn has_block(&self) -> bool {
self.block.is_some()
}

/// Adds state overrides to an existing instance.
pub fn with_state(mut self, state: StateOverride) -> Self {
self.state = Some(state);
self
}

/// Adds block overrides to an existing instance.
pub fn with_block(mut self, block: Box<BlockOverrides>) -> Self {
self.block = Some(block);
self
}
}

impl From<Option<StateOverride>> for EvmOverrides {
Copy link
Member

Choose a reason for hiding this comment

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

impl From<StateOverrides>
impl From<Box<BlockOverrides>>
impl From<Option<Box<BlockOverrides>>>

Copy link
Member

Choose a reason for hiding this comment

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

I'd actually like to remove this from impl entirely, the caller should call EvmOverrides::state instead

fn from(state: Option<StateOverride>) -> Self {
Self::state(state)
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -104,4 +163,72 @@ mod tests {
state_override.get(&address!("1b5212AF6b76113afD94cD2B5a78a73B7d7A8222")).unwrap();
assert!(acc.state_diff.is_some());
}

#[test]
fn test_evm_overrides_new() {
let state: StateOverride = HashMap::new();
let block: Box<BlockOverrides> = Box::default();

let evm_overrides = EvmOverrides::new(Some(state.clone()), Some(block.clone()));

assert!(evm_overrides.has_state());
assert!(evm_overrides.has_block());
assert_eq!(evm_overrides.state.unwrap(), state);
assert_eq!(*evm_overrides.block.unwrap(), *block);
}

#[test]
fn test_evm_overrides_state() {
let state: StateOverride = HashMap::new();
let evm_overrides = EvmOverrides::state(Some(state.clone()));

assert!(evm_overrides.has_state());
assert!(!evm_overrides.has_block());
assert_eq!(evm_overrides.state.unwrap(), state);
}

#[test]
fn test_evm_overrides_block() {
let block: Box<BlockOverrides> = Box::default();
let evm_overrides = EvmOverrides::block(Some(block.clone()));

assert!(!evm_overrides.has_state());
assert!(evm_overrides.has_block());
assert_eq!(*evm_overrides.block.unwrap(), *block);
}

#[test]
fn test_evm_overrides_with_state() {
let state: StateOverride = HashMap::new();
let mut evm_overrides = EvmOverrides::default();

assert!(!evm_overrides.has_state());

evm_overrides = evm_overrides.with_state(state.clone());

assert!(evm_overrides.has_state());
assert_eq!(evm_overrides.state.unwrap(), state);
}

#[test]
fn test_evm_overrides_with_block() {
let block: Box<BlockOverrides> = Box::default();
let mut evm_overrides = EvmOverrides::default();

assert!(!evm_overrides.has_block());

evm_overrides = evm_overrides.with_block(block.clone());

assert!(evm_overrides.has_block());
assert_eq!(*evm_overrides.block.unwrap(), *block);
}

#[test]
fn test_evm_overrides_from_state() {
let state: StateOverride = HashMap::new();
let evm_overrides: EvmOverrides = Some(state.clone()).into();

assert!(evm_overrides.has_state());
assert_eq!(evm_overrides.state.unwrap(), state);
}
}