chore: remove Endpoint and Protocol types

[sergerad]

Closes #651

• Remove Endpoint and Protocol types from util crate
• Use url::Url for handling endpoints both in TOML configuration and runtime

[Mirko-von-Leipzig]

I'm unsure when/whether we need the protocol http prefix or not?

[Mirko-von-Leipzig]

You'll also need to add a [BREAKING] changelog entry for this because it impacts configuration

[sergerad]

I'm unsure when/whether we need the protocol http prefix or not?

Yea the faucet config might be the only one that should omit it:

pub struct FaucetConfig {
    /// Endpoint of the faucet in the format `<ip>:<port>`
    pub endpoint: String,
...
            let listener = TcpListener::bind(&config.endpoint)
...

I think the rest should have the protocol specified (as per your suggested fixes). I was hoping some tests would prove me right / wrong but I haven't checked yet.

[Mirko-von-Leipzig]

I think the rest should have the protocol specified (as per your suggested fixes). I was hoping some tests would prove me right / wrong but I haven't checked yet.

I think there are no tests that actually validate the urls. We should at minimum add tests for the default settings.

This does make me wonder - maybe instead of String we should make them Url?

Then our tests for the config files and defauls would pass/fail deserialization.

[sergerad]

This does make me wonder - maybe instead of String we should make them Url?

This would be my goto approach. I can do that in the next commit.

Alternative to that (I prefer the Url type) is to have the protocol prefix in every instance of the String and do this in that socket addr flow:

let endpoint = config.endpoint.split_once("://").unwrap_or(("", &config.endpoint)).1;

[Mirko-von-Leipzig]

This does make me wonder - maybe instead of String we should make them Url?

This would be my goto approach. I can do that in the next commit.

Alternative to that (I prefer the Url type) is to have the protocol prefix in every instance of the String and do this in that socket addr flow:

let endpoint = config.endpoint.split_once("://").unwrap_or(("", &config.endpoint)).1;

Let's go with Url :) Also makes it more obvious what things are from the type. We are (probably) going to overhaul configuration in general as per #650, unless someone is violently opposed. Url would fit in nicely there as well.

[Mirko-von-Leipzig]

Impl looks good; just the question of http::Uri via tonic or url::Url. tbh I'm unsure :) Less deps is nice so maybe the former?

[sergerad]

Impl looks good; just the question of http::Uri via tonic or url::Url. tbh I'm unsure :) Less deps is nice so maybe the former?

I implemented the Uri approach in an alternate branch. I didn't push it to this branch because the last part of the impl made me think we should stick with Url instead. Url is a more accurate type for our use case here - we actually use all the values as URLs. Uri's might not have a host/port so for example the conversion to socket address involves options here:

        let socket_addr = (
            config
                .endpoint
                .host()
                .ok_or(anyhow::anyhow!("Config endpoint with no host: {}", config.endpoint))?,
            config
                .endpoint
                .port_u16()
                .ok_or(anyhow::anyhow!("Config endpoint with no port: {}", config.endpoint))?,
        );
        let rpc_listener = TcpListener::bind(socket_addr).await?;

We would want to abstract that logic behind a common trait rather than repeat it for every config that has a Uri in it. At that point we are (ironically) creating an abstraction because we introduced Uri which is a less accurate and convenient type than Url for our use case. Url naturally provides the socket_addr() fn.

This branch is ready for review again with the Url approach. LMK your thoughts and anything else to change. TY

[Mirko-von-Leipzig]

Thank you :)

Apologies about the back and forth; but I think we landed in a good spot.