Move credential verifier in peer controller

[neacsu]

This is in preparation for https://nymtech.atlassian.net/browse/VPN-3733, to not duplicate the verifier code (minus the actual verification operation, which is harder to unit test because of expiration checks)

---

This change is 

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nym-explorer-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 7, 2025 9:22am
nym-node-status	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 7, 2025 9:22am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs-nextra	⬜️ Ignored (Inspect)	Visit Preview		Aug 7, 2025 9:22am

[pronebird]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @jstuczyn)

---

service-providers/authenticator/src/peer_manager.rs line 143 at r2 (raw file):

                AuthenticatorError::InternalError("no response for topup bandwidth".to_string())
            })?;
        if !success {

Nit: perhaps you could rewrite that without negation and return:

if success {
  Ok(verifier)
} else {
  Err(AuthenticatorError::InternalError("querying peer could not be performed".to_owned()))

[pronebird]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @jstuczyn and @neacsu)

---

service-providers/authenticator/src/peer_manager.rs line 145 at r2 (raw file):

        if !success {
            return Err(AuthenticatorError::InternalError(
                "querying peer could not be performed".to_string(),

Nit: to_string() is a generic trait implemented for all Display types. I'd suggest going with .to_owned(). Some time ago it was also more performant than to_string(). Although the end result is probably the same in the context of &str, it's still semantically better. to_owned() is also used in std https://doc.rust-lang.org/stable/src/alloc/string.rs.html#2974

[pronebird]

Reviewed 2 of 7 files at r1, 2 of 4 files at r2.
Reviewable status: 4 of 8 files reviewed, 3 unresolved discussions (waiting on @jstuczyn and @neacsu)

---

common/wireguard/src/peer_controller.rs line 375 at r2 (raw file):

                            let ret = self.handle_query_verifier(&key, *credential).await;
                            if let Ok(verifier) = ret {
                                response_tx.send(QueryVerifierControlResponse { success: true, verifier: Some(verifier) }).ok();

Nit: I'd produce QueryVerifierControlResponse from if else branch and then send it once instead of sending it twice. Also the error is swallowed, does it need to be printed?

[pronebird]

Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @jstuczyn and @neacsu)

---

service-providers/authenticator/src/peer_manager.rs line 415 at r2 (raw file):

            .query_verifier(public_key, credential)
            .await
            .unwrap()

Nit: not sure it matters but you'd benefit from using expect to give context wrt which unwrap fails

[pronebird]

Reviewed 2 of 7 files at r1, 2 of 4 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @jstuczyn and @neacsu)

[neacsu]

Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @jstuczyn and @pronebird)

---

common/wireguard/src/peer_controller.rs line 375 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: I'd produce QueryVerifierControlResponse from if else branch and then send it once instead of sending it twice. Also the error is swallowed, does it need to be printed?

I'll send the entire result instead, so it can be printed by caller

---

service-providers/authenticator/src/peer_manager.rs line 143 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: perhaps you could rewrite that without negation and return:

if success {
  Ok(verifier)
} else {
  Err(AuthenticatorError::InternalError("querying peer could not be performed".to_owned()))

I've simplified it even further following your comment above

---

service-providers/authenticator/src/peer_manager.rs line 415 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: not sure it matters but you'd benefit from using expect to give context wrt which unwrap fails

Also simplified

[pronebird]

Reviewed 1 of 6 files at r3.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on @durch, @dynco-nym, and @jstuczyn)