bitcoin-dev-project / sim-ln

Payment activity generator for the lightning network
MIT License
63 stars 28 forks source link

feat: add simulation config #166

Closed enigbe closed 5 months ago

enigbe commented 10 months ago

What this PR does

Related Issue(s)

Update

Note: There are some changes to unrelated files because of the requirement to wrap maximum width of code and comments at 120 column spaces. Thought to include them regardless following conversation from #164 on maintaining width.

okjodom commented 9 months ago

@enigbe here comes a late review!

I've read through the changes proposed and seem beyond the scope of #157

Add a SimulationConfig struct that passes all the configuration options to the simulator rather than individual params.

I was expecting we create this new struct in sim-lib and update Simulation::new() to take nodes, activity, and config as params. In sim-cli we can just add a type coercion which converts the cli params into SimulationConfig and use it after Cli.parse(). Example coercion below

impl Cli {
    fn to_simulation_config(&self) -> anyhow::Result<SimulationConfig> {
        Ok(SimulationConfig {
            total_time: self.total_time.unwrap_or(60),
            print_batch_size: self.print_batch_size,
            log_level: self.log_level,
            expected_pmt_amt: self.expected_pmt_amt,
            capacity_multiplier: self.capacity_multiplier,
            no_results: self.no_results,
        })
    }
}

async fn main() {
  let cli = Cli::parse();
  let opts = cli.to_simulation_config().unwrap();
  ...  
}

Any changes that require the .ini might make sense if there's a user scenario needing it. I'm not aware of any so deferring to @carlaKC

Make log_interval configurable while we're here (currently hard set to 60s)

cool beans on this part of the issue. change lgtm :)

carlaKC commented 9 months ago

While this is outside of the scope of the original issue, I think it's a really nice extension to simln / pretty standard way to handle configuration. I can see it being useful when using simln in larger deployments where you want to have a config file checked in rather than deal with cli flags, so my vote is to keep the proposed functionality 👍

I was expecting we create this new struct in sim-lib and update Simulation::new() to take nodes, activity, and config as params.

Same here - the original issue was opened because clippy gets unhappy with the number of parameters that Simulation:new takes, so we're going to want to add a struct to sim-lib.

carlaKC commented 7 months ago

Needs rebase!

carlaKC commented 6 months ago

Since this is pretty stale now and needs a rebase anyway, let's split it out into a change for just adding a simple config struct and another for a config file - as originally flagged in early review. Would also be nice to have some of the default mechanics in place that we have for the sim file (automatically detect a default value, for example).

carlaKC commented 5 months ago

Closing due to inactivity, please feel free to re-open if you've got capacity to pick this back up with the suggested split.