Conversation
djc
changed the title
|
@Ralith I think this is ready for a first round of review! |
Ralith
left a comment
Ralith
left a comment
There was a problem hiding this comment.
Looks pretty good! I'm pleasantly surprised at how graceful this ended up being; hopefully Quinn will provide an excellent foundation for future experiments with e.g. a lightweight noise protocol based crypto layer.
It would be nice to validate the abstractions here with some non-TLS implementation, but unless someone's waiting in the wings to do that I don't think it makes sense as a blocker. We're more likely to hear from a prospective user if we have a 90% usable extension API than none at all.
Could we also make the quinn builders generic? The rustls-specific methods could be guarded with e.g. impl EndpointBuilder<rustls::TlsSession> and we'd open up potential for future conditionally-compiled methods for other cryptographic protocol implementations.
I tried some things, but if we want the high-level API to remain ergonomic, I think this gets pretty complex. Might dive into it as a follow-up, though? Seems to make sense that propagating the abstraction further out can be layered on top of this. |
|
Have another look? |
Ralith
left a comment
Ralith
left a comment
There was a problem hiding this comment.
Looks good; awesome generalization!
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
2 participants
No description provided.