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

Stream tunnel state over grpc #1924

Merged
merged 8 commits into from
Jan 16, 2025
Merged

Stream tunnel state over grpc #1924

merged 8 commits into from
Jan 16, 2025

Conversation

pronebird
Copy link
Contributor

@pronebird pronebird commented Jan 14, 2025

  • Move common types shared between nym-vpn-lib and nym-vpn-proto into nym-vpn-lib-types
  • Implement bridging uniffi types for nym-vpn-lib-types and provide From conversions. It's really inconvenient that we have to duplicate structs but I guess we can't change how uniffi works.

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 30 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @pronebird, @rokas-ambrazevicius, and @zaneschepke)


proto/nym/vpn.proto line 1034 at r1 (raw file):

}

enum ActionAfterDisconnect {

Can we scope these two? Or workaround it by prefixing the enum cases? These end up in the global namespace. You can check this by try e.g adding a INTERNAL = 4 to ActionAfterDisconnect, it will collide with INTERNAL = 10 from ErrorStateReason and fail to compile

@github-actions github-actions bot requested a review from octol January 15, 2025 09:08
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 30 files reviewed, 1 unresolved discussion (waiting on @doums, @neacsu, @octol, @rokas-ambrazevicius, and @zaneschepke)


proto/nym/vpn.proto line 1034 at r1 (raw file):

Previously, octol (Jon Häggblad) wrote…

Can we scope these two? Or workaround it by prefixing the enum cases? These end up in the global namespace. You can check this by try e.g adding a INTERNAL = 4 to ActionAfterDisconnect, it will collide with INTERNAL = 10 from ErrorStateReason and fail to compile

Thanks for raising this. I have moved both of enums into TunnelState message.

Error error = 5;
Offline offline = 6;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

neat API!

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.

:lgtm:

Reviewable status: 0 of 30 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 16, 2025 10:30
@pronebird pronebird merged commit e869b7e into develop Jan 16, 2025
14 of 15 checks passed
@pronebird pronebird deleted the am/grpc-new-state branch January 16, 2025 11:47
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.

4 participants