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

Make API requests through nym-http-api-client #1951

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

Conversation

jmwample
Copy link
Contributor

@jmwample jmwample commented Jan 17, 2025

This change is intended to reduce (ideally to 0) the number of places that we are accessing the Nym API without using the nym-http-api-client.

There were only a handful of places that this was not already the case. While there is likely a more efficient refactor that could be done to better organize the API accesses and the resources that we build out of those accesses, that is not in the scope of this PR.

There is some slight awkwardness around the async / sync function calls here. I tried to keep the existing function signatures as they were which in some places meant using a block_on to call an async fn in a sync context.


This change is Reviewable

@jmwample jmwample requested a review from octol January 17, 2025 22:54
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.

Great to see this being tidied up! Well done

Just FYI, this is the nym-vpn-api, not to be confused with the nym-api which, while it has a similar name, has a wildly different purpose

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @jmwample)


nym-vpn-core/crates/nym-vpn-network-config/src/envs.rs line 16 at r1 (raw file):

use super::{MAX_FILE_AGE, NETWORKS_SUBDIR};

// TODO: integrate with nym-vpn-api-client

Maybe this comment is now out of date?


nym-vpn-core/crates/nym-vpn-api-client/src/error.rs line 119 at r1 (raw file):

    #[error("failed to get vpn network Details")]
    FailedToGetVPNNetworkDetails(#[source] HttpClientError<UnexpectedError>),

Please use FailedToGetVpnNetworkDetails capitalization for consistency and readability


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 874 at r1 (raw file):

// Bootstrapping Environments and Network Discovery
impl VpnApiClient {

Since the bootstrap functions are intended to be used in quite separate contexts to the other functions, I wonder if it should be separate? Something like a

struct BootstrapVpnApiClient {
    inner: VpnApiClient,
}

since it's not a general purpose vpn-api-client? Then I think it will be more clear in the caller context what is going on, and it will not be possible to start calling the other methods.

And we can move it to a separate mod and file


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 878 at r1 (raw file):

    /// allowing more refined URL usage.
    // hard coded for now.
    const WELLKNOWN_URL: &str = "https://nymvpn.com/api";

This url is part of the at-build-time generated default implementation of Discovery, I wonder if it makes sense to refer to that one instead of duplicating it? I'm thinking you could pass it down the callstack?


nym-vpn-core/crates/nym-vpn-api-client/src/client.rs line 932 at r1 (raw file):

    pub async fn get_nym_vpn_network_details(&self) -> Result<NymWellknownDiscoveryItem> {
        tracing::debug!("Fetching nym vpn network details");

Just a fyi, likely better to attach a tracing::instrument to the function instead.
But I think this code was just moved as-is so I don't think it's required in this PR


nym-vpn-core/crates/nym-vpn-network-config/src/discovery.rs line 55 at r1 (raw file):

    pub fn fetch(network_name: &str) -> anyhow::Result<Self> {
        let client = VpnApiClient::well_known()?;

You can pass in the bootstrap URL here. The one that is generated as part of the Default impl

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.

2 participants