Move account controller checks into the state machine connecting state

[octol]

The first step towards integrating the account controller into the state machine is to move the readyness checks into the connecting state. That's what this PR sets out to do

Most of the code changes is to adapt to the different error types, as now the account errors will pass through the tunnel state error outcome.

The proto definitions have undergone significant changes for things related to account handling, and is split out into its own file

The impact on applications is to update to handle the changed grpc and uniffi response types.

This change is 

[pronebird]

Reviewed 9 of 12 files at r1, all commit messages.
Reviewable status: 9 of 12 files reviewed, 1 unresolved discussion (waiting on @octol)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/tunnel_state.rs line 159 at r1 (raw file):

    /// Account error
    Account,

We probably want to expand the kinds of account errors

[pronebird]

Reviewed 1 of 12 files at r1.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @octol)

[pronebird]

Reviewed 2 of 12 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @octol)

[octol]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/tunnel_state.rs line 159 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

We probably want to expand the kinds of account errors

Yep, I think we're gonna need a bunch of new error cases here. I'll start working on that once the other proto changes PR have been merged

[pronebird]

Reviewed 6 of 58 files at r2.
Reviewable status: 7 of 59 files reviewed, 3 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-account-controller/src/util.rs line 29 at r2 (raw file):

// protect us against the names drifting out of sync.

pub fn remove_files_for_account(data_dir: &Path) -> Result<(), Error> {

Nit: Utility or util is very generic name and considered anti-pattern. Perhaps this better be called something along the lines of "storage cleanup"?

---

nym-vpn-core/crates/nym-vpn-account-controller/src/util.rs line 89 at r2 (raw file):

}

pub fn into_endpoint_failure(

Would it make sense to implement TryFrom<nym_vpn_api_client::VpnApiClientError> for nym_vpn_lib_types::VpnApiErrorResponse inside of nym_vpn_lib_types and perhaps feature gate it if needed.

---

nym-vpn-core/crates/nym-vpn-account-controller/src/controller.rs line 388 at r2 (raw file):

    async fn unregister_device_from_api(&self) -> Result<NymVpnDevice, AccountCommandError> {
        tracing::info!("Unregistering device from API");
        if self.shared_state().ready_to_register_device().await == ReadyToRegisterDevice::InProgress

FYI maybe down the road this request could be enqueued to avoid erroring.

[pronebird]

Reviewed 24 of 58 files at r2.
Reviewable status: 31 of 59 files reviewed, 3 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/tunnel_state.rs line 163 at r2 (raw file):

    /// Account related error not specifically handled.
    Account(String),

Is it some kind of internal error?

Since we started adding nested errors, what do you think about grouping all these kinds under three enum variants?

• Account errors
• Device errors
• ZkNym errors

This way we could keep the top level list a bit more concise.

[pronebird]

Reviewable status: 31 of 59 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/mod.rs line 15 at r2 (raw file):

    // Catch all for any error not specifically handled
    #[error("general error: {0}")]
    General(String),

Maybe we should merge general into internal errors? Feels like both are of more or less the same kind.

[pronebird]

Reviewed 1 of 58 files at r2.
Reviewable status: 32 of 59 files reviewed, 5 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/forget_account.rs line 30 at r2 (raw file):

    #[error("failed to init device keys: {0}")]
    InitDeviceKeys(String),

Is this error variant really used?

[pronebird]

Reviewed 27 of 58 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

[octol]

Reviewable status: 48 of 59 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/tunnel_state.rs line 163 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Is it some kind of internal error?

Since we started adding nested errors, what do you think about grouping all these kinds under three enum variants?

• Account errors
• Device errors
• ZkNym errors

This way we could keep the top level list a bit more concise.

thinking ...

Currently they are grouped per operation. Requesting a zknym might fail due to an account error. It's basically top down vs bottom up grouping. I think there are good reasons for both. Let me sleep on it first

If you go into an unrecoverable error state, doesn't it make sense for the error to correspond to the high-level action that failed? E.g "Device registration failed" (this grouping) instead of e.g "Account not registered" (the proposed grouping)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/forget_account.rs line 30 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Is this error variant really used?

It's when we fail to reinit new device identity keys in storage when forgetting the existing account

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/mod.rs line 15 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Maybe we should merge general into internal errors? Feels like both are of more or less the same kind.

In my head Internal is something more along the lines of an assertion failed. An internal error happens due to a bug vs "General" being mostly just lazily grouping some uncommon error cases

Looking at what's triggering them Internal is mostly things like sending on unbounded channel failed and similar, while General are more normal errors originating to storage or api request failures

---

nym-vpn-core/crates/nym-vpn-account-controller/src/util.rs line 29 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: Utility or util is very generic name and considered anti-pattern. Perhaps this better be called something along the lines of "storage cleanup"?

Renamed

---

nym-vpn-core/crates/nym-vpn-account-controller/src/util.rs line 89 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Would it make sense to implement TryFrom<nym_vpn_api_client::VpnApiClientError> for nym_vpn_lib_types::VpnApiErrorResponse inside of nym_vpn_lib_types and perhaps feature gate it if needed.

It does indeed!

[octol]

Reviewable status: 48 of 59 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/mod.rs line 15 at r2 (raw file):

Previously, octol (Jon Häggblad) wrote…

In my head Internal is something more along the lines of an assertion failed. An internal error happens due to a bug vs "General" being mostly just lazily grouping some uncommon error cases

Looking at what's triggering them Internal is mostly things like sending on unbounded channel failed and similar, while General are more normal errors originating to storage or api request failures

Been going over this again, and the errors that are mapped into General don't really happen in the commands run from the tunnel, so I think there is some merit to this

[octol]

I reworked the errors returned, to be narrower so that only errors that can happen inside the tunnel are mapped to tunnel state error

Reviewable status: 28 of 59 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

[octol]

Reviewable status: 28 of 59 files reviewed, 4 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/tunnel_state.rs line 163 at r2 (raw file):

Previously, octol (Jon Häggblad) wrote…

thinking ...

Currently they are grouped per operation. Requesting a zknym might fail due to an account error. It's basically top down vs bottom up grouping. I think there are good reasons for both. Let me sleep on it first

If you go into an unrecoverable error state, doesn't it make sense for the error to correspond to the high-level action that failed? E.g "Device registration failed" (this grouping) instead of e.g "Account not registered" (the proposed grouping)

Ok that error is gone now

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/mod.rs line 15 at r2 (raw file):

Previously, octol (Jon Häggblad) wrote…

Been going over this again, and the errors that are mapped into General don't really happen in the commands run from the tunnel, so I think there is some merit to this

Updated

[pronebird]

Reviewed 1 of 11 files at r3.
Reviewable status: 29 of 59 files reviewed, 3 unresolved discussions (waiting on @doums, @neacsu, @rokas-ambrazevicius, and @zaneschepke)

[pronebird]

Reviewed 1 of 11 files at r3, 2 of 26 files at r4.
Reviewable status: 32 of 59 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @rokas-ambrazevicius, and @zaneschepke)

[pronebird]

Reviewed 3 of 11 files at r3, 16 of 26 files at r4, 1 of 1 files at r5.
Reviewable status: 51 of 59 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @rokas-ambrazevicius, and @zaneschepke)

[pronebird]

Reviewed 8 of 26 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @rokas-ambrazevicius, and @zaneschepke)

[octol]

Reviewable status: 52 of 59 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

---

nym-vpn-core/crates/nym-vpn-lib-types/src/account/mod.rs line 15 at r2 (raw file):

Previously, octol (Jon Häggblad) wrote…

Updated

Removed General from the error enum

[pronebird]

Reviewed 7 of 7 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @doums, @neacsu, @rokas-ambrazevicius, and @zaneschepke)