Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change] Add conversions from proto to core types #1950

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

pronebird
Copy link
Contributor

@pronebird pronebird commented Jan 17, 2025

  1. Add From conversions from proto to TunnelState
  2. Simplify types used in TunnelState by converting some of the nym types into String
  3. nym-lib-types: Add feature gate nym-type-conversions to remove dependendencies on nym crates when not needed.
  4. Drop some of the old types and return TunnelState from grpc which is a breaking change and it will break desktop apps.
  5. nym-vpnc: Leverage From conversions instead of working with raw grpc types when reporting vpn status.

This change is Reviewable

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 24 files reviewed, 2 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)


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

pub struct ConnectionData {
    /// Mixnet entry gateway identity encoded as base58 string.
    pub entry_gateway: String,

I kinda think we should have strong types for identity and address, even if it's just a thin wrapper around their base58 string representations. I've observed these types being easy to mix up for newcomers to the code base

@github-actions github-actions bot requested a review from octol January 21, 2025 09:39
Copy link
Contributor Author

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 25 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)


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

Previously, octol (Jon Häggblad) wrote…

I kinda think we should have strong types for identity and address, even if it's just a thin wrapper around their base58 string representations. I've observed these types being easy to mix up for newcomers to the code base

Added Gateway and NymAddress wrapper types.

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


nym-vpn-core/crates/nym-vpn-lib-types/src/connection_data.rs line 86 at r4 (raw file):

        Self::new(value.gateway().to_base58_string())
    }
}

I like this, it establishes a compile time link between the types and what the string represents

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 17 of 24 files at r1, 1 of 1 files at r2, 2 of 7 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)

@github-actions github-actions bot requested review from doums and octol January 22, 2025 09:15
Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)


nym-vpn-core/crates/nym-vpn-lib-types/src/connection_data.rs line 84 at r4 (raw file):

impl From<nym_gateway_directory::Recipient> for NymAddress {
    fn from(value: nym_gateway_directory::Recipient) -> Self {
        Self::new(value.gateway().to_base58_string())

This here gives you the gateway identity sub-part of the nym-address, not the nym-address itself

@github-actions github-actions bot requested a review from octol January 22, 2025 09:31
Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


nym-vpn-core/crates/nym-vpn-lib-types/src/connection_data.rs line 84 at r4 (raw file):

Previously, octol (Jon Häggblad) wrote…

This here gives you the gateway identity sub-part of the nym-address, not the nym-address itself

Looking good!

Copy link
Contributor Author

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @doums from a discussion.
Reviewable status: 24 of 26 files reviewed, all discussions resolved (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)

@pronebird pronebird requested a review from octol January 22, 2025 09:48
@pronebird
Copy link
Contributor Author

Addressed all concerns.

Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions bot requested a review from octol January 22, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants