Implement separate types for client and server TransportParameters

[djc]

The current API is pretty confusing because it requires passing in a Side, but it's not obvious if that is the API user's side or the message originator's side. My original implementation had explicit ClientTransportParameters and ServerTransportParameters types, which is more obvious, and can also directly implement the Value/Codec trait.

[jansol]

I'd like to give this a try

[djc]

Sounds great! You can have a look here at my original code for this:

https://github.com/djc/quinn/blob/pre-quicr-quinn/src/parameters.rs

(Note that Value is now called Codec, and you'll have to fiddle with the error handling if you want to use it. I think it would be nice to have Codec::decode() return DecodeError which will include the current UnexpectedEnd, which can then contain a variant TransportParametersError which contains a transport_parameters::Error. @Ralith does that work for you?)

[Ralith]

Codec as it stands is used for things that have no invalid representations, which makes its use simple and allows for a zero-size error type. I'm not a huge fan of introducing additional error cases for the sake of a single type. Is there a good reason not to use inherent methods/associated functions?

[djc]

Okay, I'm fine using just simple methods on the new types.

[djc]

@jansol are you still interested in finishing this? If not, someone else might want to take a swing!

[jansol]

Yeah, just had a quick chat with Ralith yesterday about this.

[djc]

Okay, good!

[Ralith]

Note that this code has been significantly refactored in the quic-15 branch; work on this issue should be done relative to those changes rather than master.

[jansol]

Did you mean quic-15 or draft-15? Because I did rebase after draft-15 was merged.

[Ralith]

I meant draft-15, which has not yet been merged to master (but to which the refactored transport parameters were merged).

[djc]

This has been rendered irrelevant by the changes in draft 19.

[Ralith]

I'm not certain about that; there's still a number of server-specific fields we're currently representing with Option.

[lmpn]

I finding it hard to understand what needs to be changed... The details of the issue are up to date or this issue is no longer valid?

[Ralith]

TransportParameters contains three fields that are never set by clients, and always set by servers. This could be expressed a little more precisely by having two types, one in which they're all required, and one in which server-specific fields do not exist at all.

That said, it's not obvious that this would lead to simpler/more robust code, which is the real goal. I lean towards closing this as not planned. @djc, any thoughts?

[djc]

Yeah, agreed that there's probably not much value here. @lmpn I'm guessing you may have gotten here via the Hacktoberfest/good first issue labels, @Ralith do you have any other (more current) ideas for something like that?

[Ralith]

I see you've just gone through and added a bunch of reasonable things; I'll watch for other opportunities as well. https://github.com/quinn-rs/quinn/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22 is also good.