Entry wireguard tickets #4888
Entry wireguard tickets #4888
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
There was a problem hiding this comment.
Looking good!
Reviewed 20 of 35 files at r1, 21 of 21 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jstuczyn and @neacsu)
common/authenticator-requests/src/error.rs line 1 at r3 (raw file):
// Copyright 2023 - Nym Technologies SA <contact@nymtech.net>
super duper nitpick: 2024
common/authenticator-requests/src/v2/conversion.rs line 9 at r3 (raw file):
fn from(authenticator_request: v1::request::AuthenticatorRequest) -> Self { Self { version: 2,
Should we take this opportunity to move the Authenticator messages to it's own dedicated band of version numbers? Possibly 2 -> 102? Or we just skip that and add another tag indicating the protocol (ipr, authenticator)?
service-providers/authenticator/src/mixnet_listener.rs line 256 at r3 (raw file):
.remove_peer(&final_message.gateway_client) .await .tap_err(|err| {
Can do this with inspect_err nowadays, so no need for the tap crate
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jstuczyn and @octol)
common/authenticator-requests/src/v2/conversion.rs line 9 at r3 (raw file):
Previously, octol (Jon Häggblad) wrote…
Should we take this opportunity to move the Authenticator messages to it's own dedicated band of version numbers? Possibly
2 -> 102? Or we just skip that and add another tag indicating the protocol (ipr, authenticator)?
I think it's worth including the tag in this one, why not. As a note, it's more important on the response side, as the client could (currently it doesn't) be multiplexing the mixnet connection. On the request side, it should be separated because of the nym address factor, but for consistency, I will add it to both entry and exit.
I'm thinking to add this protocol tag (with version and type) for this version just for authenticator, with a common struct that could be used by ipr and nr later respectively, when there are individual version changes for them, in their respective PRs. That way it wouldn't side track this PR.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jstuczyn and @neacsu)
common/authenticator-requests/src/v2/conversion.rs line 9 at r3 (raw file):
Previously, neacsu (Bogdan-Ștefan Neacşu) wrote…
I think it's worth including the tag in this one, why not. As a note, it's more important on the response side, as the client could (currently it doesn't) be multiplexing the mixnet connection. On the request side, it should be separated because of the nym address factor, but for consistency, I will add it to both entry and exit.
I'm thinking to add this protocol tag (with version and type) for this version just for authenticator, with a common struct that could be used by ipr and nr later respectively, when there are individual version changes for them, in their respective PRs. That way it wouldn't side track this PR.
Yep sounds like a good plan!
|
The behaviour of the nodes and vpn client (as a test) has not changed, it still works as it used to. Obtaining ticketbooks also is unchanged |
This change is