EspressoSystems / espresso-sequencer

86 stars 56 forks source link

Refactor the constructors of TestNetwork #1603

Closed ImJeremyHe closed 1 week ago

ImJeremyHe commented 3 weeks ago

Currently we use this constructor a lot:

        pub async fn new(
            opt: Options,
            persistence: [impl PersistenceOptions<Persistence = P>; TestConfig::NUM_NODES],
            l1: Url,
            builder_port: Option<u16>,
        ) -> Self

l1 and builder_port here are used to set a different value into the default config. This would be troublesome if we keep adding parameters in this constructor. For example, we need a TestNetwork with a specific relay-state-url, so this constructor will be like:

        pub async fn new(
            opt: Options,
            persistence: [impl PersistenceOptions<Persistence = P>; TestConfig::NUM_NODES],
            l1: Url,
            builder_port: Option<u16>,
            relay_state_url: Option<Url>,
        ) -> Self

And every time we change the constructor, we need to do a minor fix on the other places where it is being used. This is also not a good idea for reviewing.

So I prefer using a constructor like this:

        pub async fn new_with_config(
            opt: Options,
            persistence: [impl PersistenceOptions<Persistence = P>; TestConfig::NUM_NODES],
            config: TestConfig,
        ) -> Self

We can invoke the TestNetwork after we set the config. It is clear and more elegant. People will easily know how the TestNetwork looks like.

Also we can use the builder pattern to construct a TestConfig. That would be like:

let cfg = TestConfigBuilder::new()
    .defualt()
    .l1_url("http://localhost:8545")
    .builder_port(40000)
    .build();
let network = TestNetwork::new(cfg);