AgentConfig API is error prone

[roman-kashitsyn]

Currently, AgentConfig is a bare struct with public fields implementing Default. This has 2 issues:

1. Any changes in the Config will break existing user programs.
2. The URL field is initialized with an invalid value to force user to specify it. If user makes a mistake, they will only discover it at runtime. We can easily prevent this mistake at compile time by making the fields pub(crate), removing Default and introducing a Builder initialized with required arguments, e.g.

pub struct AgentConfig { pub(crate) url: Url, ... }

impl AgentConfig {
    pub fn url(&self) -> &Url { ... }
    // ...
}

pub struct ConfigBuilder { ... }
impl ConfigBuilder {
    // Arguments of the [new] function are required.
    pub fn new(url: Url) -> Self { ... }
    // ...
    pub fn build(self) -> AgentConfig { ... }
}

[hansl]

Thanks for the feedback. The builder itself already exists; https://github.com/dfinity/agent-rs/blob/next/ic-agent/src/agent/builder.rs#L4 (and is used in icx and dfx).

We could have the url field in the Config type be Option and on build() verify it's not None. Then limit the Config visibility. This would be a breaking change but it's a very good one.

This is fairly simple work, if you want to go ahead please open a PR. I'll probably be able to find some time later this week to fix this.

[roman-kashitsyn]

The builder itself already exists

Indeed, thanks for the pointer! Then I don't quite understand why Agent::new and AgentConfig are public at all.

We could have the url field in the Config type be Option and on build() verify it's not None.

I'm not sure it's much different from the current state of affairs, the error will be still a runtime one, not compile time.

[hansl]

why Agent::new and AgentConfig are public at all.

Some historical reason, maybe a change in dfx or something. There was initially no builder, but now that there is a good one they shouldn't be public.

I'm not sure it's much different from the current state of affairs, the error will be still a runtime one, not compile time.

Right you are. Your way is better then. Thanks!