dfinity / dfx-extensions

Source repo for DFX extensions binaries and metadata
Apache License 2.0
8 stars 5 forks source link

fix: use localhost:8080 in nns-dapp init args #129

Closed mraszyk closed 2 months ago

mraszyk commented 2 months ago

This MR fixes the URLs in the nns-dapp init args to always point to localhost:8080 since the nns extension assumes that there's an HTTP gateway listening at localhost:8080 and pointing to the local replica (nns_url) breaks the nns-dapp if a user restarts the replica without a pinned replica port (since the replica port likely changes and the hard-coded URLs for the stopped replica become invalid).

anchpop commented 2 months ago

I didn’t implement any of the NNS extension and I have no idea how it works or what it does. However I don’t think anyone else on the NNS team does either so I’ve approved it to unblock you. I'd prefer if you got separate approval from someone on the SDK team who has context about the NNS extension

krpeacock commented 2 months ago

I haven't touched this code before, but I don't think that hardcoding the replica port to 8080 is a good idea. The local http gateway is configurable by dfx and we can't count on port 8080 being used or even free as a given.

Unless the nns extension spins up its own local server, I think there's a better solution here

ericswanson-dfinity commented 2 months ago

At one point we were told that the nns dapp's requirement that the replica use a fixed port of 8080 was a short-term limitation, to be addressed after launch. For example, it's not great that developers have to configure their local network in order for the nns dapp to work locally. Can we instead consider what it would take to remove this limitation?

mraszyk commented 2 months ago

Can we instead consider what it would take to remove this limitation?

If we could learn the webserver port here, then we could just use instead of 8080 when defining let nns_dapp_metadata. How about doing this in a separate MR though?

ericswanson-dfinity commented 2 months ago

Can we instead consider what it would take to remove this limitation?

If we could learn the webserver port here, then we could just use instead of 8080 when defining let nns_dapp_metadata. How about doing this in a separate MR though?

Replica port is available here, or call dfx info replica-port: https://github.com/dfinity/sdk/blob/master/src/dfx/src/commands/info/replica_port.rs

But it's true that if the replica restarts, it will come up on a different port. I wonder if it would work to use the proxy port (available here) instead, which is fixed, since it forwards non-api requests to the replica.

Either way, a separate PR sounds fine.

mraszyk commented 2 months ago

Replica port is available here, or call dfx info replica-port:

We can't use the replica port in the nns-dapp init args as some of the URL need to target the webserver (HTTP gateway, e.g., icx-proxy).

I wonder if it would work to use the proxy port (available here) instead, which is fixed, since it forwards non-api requests to the replica.

Yes, this is what I meant to use in a follow-up MR, if we could extract the webserver port from

    network: &NetworkDescriptor,
    networks_config: &NetworksConfig,

which is what we have around when defining the nns-dapp init args.

ericswanson-dfinity commented 2 months ago

if we could extract the webserver port

We can, it's network.local_server_descriptor()?.bind_address.port();