Remove old free credential handle

[neacsu]

and create some traits for unit testing purposes. Also added a few unit tests, but more will follow in future PRs

---

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	Jul 2, 2025 0:10am
nym-next-explorer	❌ Failed (Inspect)			Jul 2, 2025 0:10am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs-nextra	⬜️ Ignored (Inspect)	Visit Preview		Jul 2, 2025 0:10am

[jstuczyn]

it's always amazing to see so much removed code and additional tests!

just two questions that bother me:

• I've seen you removed the logic to do with deciding if storage peer bandwidth should be synced to the storage - we're not doing this on every update now, are we?
• given the removal of the free cap, how will it affect backwards compatibility? as in, how many clients do we expect might break because of this change?

[neacsu]

• I've seen you removed the logic to do with deciding if storage peer bandwidth should be synced to the storage - we're not doing this on every update now, are we?

The storage for peer bandwidth is removed altogether since it wasn't really needed. The kernel peer structure couldn't be populated with the restored values from disk anyway, so on gateway restart we just start from 0 the counting of what's to be consumed. The remaining update to storage is now only the one we already have from the BandwidthStorageManager, which tracks top-ups and consumption, so that we can get available bandwidth.

• given the removal of the free cap, how will it affect backwards compatibility? as in, how many clients do we expect might break because of this change?

The free cap is officially dropped since a long time ago, when we released the paid product, and such stale clients are not supported officially anymore. But let me check if we know about any old clients being used in production.

[pronebird]

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

---

common/gateway-storage/src/traits.rs line 153 at r5 (raw file):

    ) -> Result<i64, GatewayStorageError>;

    /// Tries to retrieve available bandwidth for the particular peer.

This does not seem right. The method returns WireguardPeer

[pronebird]

Reviewed 1 of 47 files at r1.
Reviewable status: 12 of 48 files reviewed, 9 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

[pronebird]

Reviewable status: 12 of 48 files reviewed, 10 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

common/gateway-storage/src/models.rs line 126 at r5 (raw file):

            allowed_ips: bincode::Options::serialize(make_bincode_serializer(), &value.allowed_ips)
                .map_err(|e| {
                    crate::error::GatewayStorageError::TypeConversion(format!("allowed ips {e}"))

Not:

I'd add two variants instead and map them and let the error printer do the rest:

#[error("Serialization failure for {field_key})]
Serialize { field_key: &'static str, source: bincode::Error },

#[error("Deserialization failure for {field_key})]
Deserialize { field_key: &'static str, source: bincode::Error },

[pronebird]

Reviewed 4 of 47 files at r1, 1 of 7 files at r5.
Reviewable status: 17 of 48 files reviewed, 11 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

common/wireguard/src/peer_controller.rs line 471 at r5 (raw file):

    }

    // #[tokio::test]

Should we not delete the dead and commented out code from our repository? In the end we have git history to resurrect it if needed?

[pronebird]

Reviewable status: 17 of 48 files reviewed, 12 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

common/wireguard/src/peer_handle.rs line 148 at r5 (raw file):

            .get(&self.public_key)
            .ok_or(Error::MissingClientKernelEntry(self.public_key.to_string()))?
            .into();

Nit: a bit difficult to say what .into() does here and as such .map(ExpectedType::from) would give a better insight. I understand that inlay type hints probably infer it properly in the IDE that's why this is a nit comment.

[pronebird]

Reviewed 1 of 47 files at r1.
Reviewable status: 18 of 48 files reviewed, 13 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

common/wireguard/src/peer_storage_manager.rs line 28 at r5 (raw file):

}

pub struct CachedPeerManager {

Would it not be better to remove this type and work with Option<PeerInformation> directly at the call site? It seems that this type is superficial

[pronebird]

Reviewed 1 of 47 files at r1.
Reviewable status: 19 of 48 files reviewed, 14 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

gateway/src/node/mod.rs line 40 at r5 (raw file):

pub use client_handling::active_clients::ActiveClientsStore;
pub use nym_gateway_stats_storage::PersistentStatsStorage;
pub use nym_gateway_storage::{error::GatewayStorageError, traits::*, GatewayStorage};

Perhaps this wildcard is not needed? Also would it not be better to explicitly import what needs to be in the scope?

[neacsu]

Reviewable status: 19 of 48 files reviewed, 14 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, @pronebird, and @simonwicky)

---

common/credential-verification/src/ecash/state.rs line 25 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I wonder if using Arc<RwLock<SharedState>> would remove the need for dyn_clone? I don't know much about this code but this is something that

I think this might be a question for @simonwicky . Is there a reason for putting RwLocks inside the shared state instead of on the entire state? Performance or functional reasons? I see this complicates things a bit with guards all over the place and RwLockReadGuard & RwLockWriteGuard returned by some functions, would it be possible/better to just rw lock the entire structure?
I'm a bit reluctant to change things here functionally so that we don't end up with some unexpected dead locks.

---

common/gateway-storage/src/models.rs line 126 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Not:

I'd add two variants instead and map them and let the error printer do the rest:

#[error("Serialization failure for {field_key})]
Serialize { field_key: &'static str, source: bincode::Error },

#[error("Deserialization failure for {field_key})]
Deserialize { field_key: &'static str, source: bincode::Error },

Done.

---

common/gateway-storage/src/traits.rs line 153 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This does not seem right. The method returns WireguardPeer

Done.

---

common/gateway-storage/src/traits.rs line 207 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

A dozen of fields use Arc<RwLock<_>>. Would it not be better to use one Arc<RwLock<MockGatewayStorage>> instead?

That's a less verbose way of implementing it as well. I'll apply the changes, thanks for the suggestion.

---

common/wireguard/src/peer_controller.rs line 471 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Should we not delete the dead and commented out code from our repository? In the end we have git history to resurrect it if needed?

This is a test I was thinking adding here initially, but it seems it worked better in the service provider crate. I'll remove the commented code

---

common/wireguard/src/peer_storage_manager.rs line 28 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Would it not be better to remove this type and work with Option<PeerInformation> directly at the call site? It seems that this type is superficial

I find it easier to follow when being wrapped, although the exact reason for this wrapper is the previous implementation.

---

gateway/src/node/mod.rs line 40 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Perhaps this wildcard is not needed? Also would it not be better to explicitly import what needs to be in the scope?

Done.

[simonwicky]

Reviewable status: 17 of 49 files reviewed, 14 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, @neacsu, and @pronebird)

---

common/credential-verification/src/ecash/state.rs line 25 at r5 (raw file):

Previously, neacsu (Bogdan-Ștefan Neacşu) wrote…

I think this might be a question for @simonwicky . Is there a reason for putting RwLocks inside the shared state instead of on the entire state? Performance or functional reasons? I see this complicates things a bit with guards all over the place and RwLockReadGuard & RwLockWriteGuard returned by some functions, would it be possible/better to just rw lock the entire structure?
I'm a bit reluctant to change things here functionally so that we don't end up with some unexpected dead locks.

While git blame has my name on it, it's a part of one of the "Grand ecash squasheroos" that @jstuczyn did, so I have to call him on the stand for that, as I can't answer that question

[jstuczyn]

common/credential-verification/src/ecash/state.rs line 25 at r5 (raw file):

Previously, simonwicky (Simon Wicky) wrote…

While git blame has my name on it, it's a part of one of the "Grand ecash squasheroos" that @jstuczyn did, so I have to call him on the stand for that, as I can't answer that question

it's been a while so I can't answer with 100% certainty, but I looked at the code history (including coconut iteration) and I think this is just result of evolving code without refactoring it.

i.e. initially it was just 2 strings and a client behind a lock (so no need to put the whole thing behind a lock). then additional 2 fields were added, both put behind their own locks. but in this instance it made sense as they were being accessed and updated independently and often in paralellel. and now we're at the current iteration.

there is still some benefit for putting data and client behind separate locks (as both of them could be accessed simultanously), but if it were to simplify the code, I think the benefit of sharing it would outweight it. However, if you're going to be doing refactoring, please make sure any chain transaction you make with the client uses WRITE lock (even though type signature would allow you to get away with read). if you don't you will mess up your chain sequence numbers and some tx will fail.

[neacsu]

common/credential-verification/src/ecash/state.rs line 25 at r5 (raw file):

Previously, simonwicky (Simon Wicky) wrote…
it's been a while so I can't answer with 100% certainty, but I looked at the code history (including coconut iteration) and I think this is just result of evolving code without refactoring it.

i.e. initially it was just 2 strings and a client behind a lock (so no need to put the whole thing behind a lock). then additional 2 fields were added, both put behind their own locks. but in this instance it made sense as they were being accessed and updated independently and often in paralellel. and now we're at the current iteration.

there is still some benefit for putting data and client behind separate locks (as both of them could be accessed simultanously), but if it were to simplify the code, I think the benefit of sharing it would outweight it. However, if you're going to be doing refactoring, please make sure any chain transaction you make with the client uses WRITE lock (even though type signature would allow you to get away with read). if you don't you will mess up your chain sequence numbers and some tx will fail.

This is the reason I would refrain from doing those changes in this (already big) PR @pronebird, as unexpected things might break

[pronebird]

Reviewed 1 of 47 files at r1, 3 of 6 files at r6.
Reviewable status: 20 of 49 files reviewed, 9 unresolved discussions (waiting on @durch, @dynco-nym, @jstuczyn, and @neacsu)

---

common/wireguard/src/peer_storage_manager.rs line 28 at r5 (raw file):

Previously, neacsu (Bogdan-Ștefan Neacşu) wrote…

I find it easier to follow when being wrapped, although the exact reason for this wrapper is the previous implementation.

It looks strange but I will leave it up to you

Deferred