E2E: wallet, transaction and chain improvements

[loocapro]

E2E improvements:

• Wallet generator: Inspired from foundry, each wallet can create different type of transactions via the TransactionTestContext
• TransactionTestContext: refactored naming and the way the transaction was built
• TestNodeGenerator and TestNetworkBuilder abstractions: helpers to easily create nodes and perform actions on them (connecting)
• ActionRunner: implement a Stream like the tokio stream mock to push an action, await it and check it

[mattsse]

okay, I can see how this will look like,

the ChainRunner and action trait need a bit more fine tuning wrt what's being stored etc.

the core logic should just be abstracted over the trait's types.

where the magic then comes in is with impls ov ChainRunner for certain Action/Ctx types, this is where we then can add sugar for things like FnOnce() -> Future etc

[joshieDo]

looks real good overall!

i guess this can be an opinated topic, with no right or wrong way, so I'll just leave my opinion.

• Scenarios should have as little boilerplate as possible. I want to be able to read it, without being "polluted" with boilerplate/setup code. If something is repeated in both optimism, and ethereum it should be moved to e2e-utils. If something is repeated across scenarios of the same node, then it should be abstracted away in the same node utils. That's why I had added setup & advance_many before. I've even thought about static functions for generation of nodes just so we don't do the ugly nodes.pop().

Things like this are being repeated over and over, in all scenarios across all nodes which in my opinion is unnecessary

(...)
    let tasks = TaskManager::current();
    let exec = tasks.executor();
(...)

I'd be curious on how you'd implement the scenario from 7552 PR which will (hopefully) be merged into main soon

[loocapro]

looks real good overall!

Thank you, Josh, and I appreciate the time you’ve invested in the reviews—it's incredibly helpful!

i guess this can be an opinated topic, with no right or wrong way, so I'll just leave my opinion.

• Scenarios should have as little boilerplate as possible. I want to be able to read it, without being "polluted" with boilerplate/setup code. If something is repeated in both optimism, and ethereum it should be moved to e2e-utils. If something is repeated across scenarios of the same node, then it should be abstracted away in the same node utils. That's why I had added setup & advance_many before. I've even thought about static functions for generation of nodes just so we don't do the ugly nodes.pop().

Things like this are being repeated over and over, in all scenarios across all nodes which in my opinion is unnecessary

(...)
    let tasks = TaskManager::current();
    let exec = tasks.executor();
(...)

Since the TaskManager manages the current tokio main task, it must remain in scope throughout the test; otherwise, if it gets dropped, the entire environment would be affected as well. Previously, we handled this with code like:

let (mut nodes, _tasks, _wallet) = setup(2).await?;

Here, we returned the _tasks and effectively ignored them. I also wasn’t keen on the implicit naming of the setup function, which, while avoiding code duplication, obscures the underlying operations with a generic name, leaving unclear what it specifically does.

So, I explored a different design, which admittedly leaves some boilerplate:

let tasks = TaskManager::current();
let exec = tasks.executor();

However, this approach is more explicit and clarifies exactly what is happening during setup:

let mut nodes = TestNetworkBuilder::<OptimismNode>::new(2, chain_spec, exec).build().await?;

While I strive to apply the DRY (Don’t Repeat Yourself) principle wherever practical, I generally prefer explicit coding over obscuring details just to avoid duplicating logic. This stance is simply my personal preference, and I’m always eager to learn from differing viewpoints.

I'd be curious on how you'd implement the scenario from 7552 PR which will (hopefully) be merged into main soon

I guess we'll find out soon! I plan to rebase this PR on your commits once they are merged into the main branch. 🙃

[joshieDo]

I generally prefer explicit coding over obscuring details just to avoid duplicating logic

I feel like there must be nuance here. Any abstraction is obscuring details by default. However, we are "obscuring" boilerplate code. In the context of building a readable test scenario, it adds no value being explicit like this imo. I'm horrible at naming, maybe setup is not the best.

Here, we returned the _tasks and effectively ignored them

It needs to be in scope like you mentioned.

[mattsse]

this is great!

have a few nits and this needs a rebase because some types changed, should be trivial though