testing: adds a full test harness #179
Conversation
- adds a Flashbots provider with the alloy MEV API extensions - adds the logic for building Flashbots bundles in the builder - adds the Flashbots submission task
dylanlott
changed the title
| constants: SignetSystemConstants::pecorino(), | ||
| }; | ||
| Ok(config) | ||
| Ok(pecorino_config) |
There was a problem hiding this comment.
the test config is not a pecorino config. if we want a pecorino config it should be a separate setupd function
There was a problem hiding this comment.
unit and integration tests should generally NOT be run against pecorino. the test config is for local uni ttesting
| use trevm::revm::{context::BlockEnv, context_interface::block::BlobExcessGasAndPrice}; | ||
|
|
||
| /// Sets up a block builder with test values | ||
| pub fn setup_test_config() -> Result<BuilderConfig> { |
There was a problem hiding this comment.
we should base this branch on the config changes in #174
prestwich
left a comment
prestwich
left a comment
There was a problem hiding this comment.
i think the harness needs design work. Can you spec out its interface and behavior a bit more?
also, we should rebase work onto #174
Some open design questions:
- why anvil instances at all? what are we hoping to learn/control, that we can't with local dummy envs?
- the anvil instances don't seem to be fork wrapping pecorino based on the anvil instantiation. why are we using pecorino test values anywhere?
- why are slot times changing? this is fundamental to the builder and will definitely cause unintended behavior.
- the harness is "simulating" an env task. why not just make an env task?
| @@ -0,0 +1,307 @@ | |||
| //! Test harness for end-to-end simulation flow. | |||
There was a problem hiding this comment.
our style for this is to have it in tests/common/mod.rs
| constants: SignetSystemConstants::pecorino(), | ||
| }; | ||
| Ok(config) | ||
| Ok(pecorino_config) |
There was a problem hiding this comment.
unit and integration tests should generally NOT be run against pecorino. the test config is for local uni ttesting
| ) -> Result<TxEnvelope> { | ||
| let tx = TxEip1559 { | ||
| chain_id: 11155111, | ||
| chain_id: pecorino::RU_CHAIN_ID, |
There was a problem hiding this comment.
while we're fixing this, let's read it from the config by basing this work on #174 and using crate::config()
| sync::Arc, | ||
| time::{Duration, SystemTime, UNIX_EPOCH}, | ||
| }; | ||
|
|
| } | ||
|
|
||
| /// Tick a new `SimEnv` computed from the current latest rollup and host headers. | ||
| pub async fn tick_from_headers(&self, prev_ru_header: Header, prev_host_header: Header) { |
There was a problem hiding this comment.
this is now duplicating env construction code from the env task. is there a reason we're not using an EnvTask here? it seems like we could easily have one running but control anvil to only produce WS notifications when a tick function is called
| } | ||
|
|
||
| /// Receive the next SimResult with a timeout. | ||
| pub async fn recv_result(&mut self, timeout: Duration) -> Option<SimResult> { |
There was a problem hiding this comment.
the basic version of the api shouldn't include a timeout. that can be added by the caller or extended with a recv_with_timeout function
| } | ||
|
|
||
| /// Returns a host provider configured with the builder's wallet and blob gas params | ||
| async fn host_provider(&self) -> HostProvider { |
There was a problem hiding this comment.
is this duplicative with the config connection logic?
| } | ||
| } | ||
|
|
||
| // This function sets the slot timing to start now with a 10 second slot duration for tests. |
| pub async fn new() -> eyre::Result<Self> { | ||
| setup_logging(); | ||
| let mut config = setup_test_config()?; | ||
| configure_slot_timing(&mut config)?; |
There was a problem hiding this comment.
this is an anti-pattern. if we want the config to have a specific value, we should set it up that way. i;m not sure why we want to change this at all. we should be using either the test value in constants, or the pecorino value in constants
testing: adds a full test harness
This PR adds a test harness to the builder. This test harness will facilitate deeper block lifecycle testing and assertions around smart contract calls and simulation between host and rollup chains.