feat: dfx canister url #3346

krpeacock · 2023-09-01T19:48:12Z

Description

Adds a new canister subcommand - dfx canister url

Prints the URL of a canister

Usage: dfx canister url [OPTIONS] <CANISTER>

Arguments:
  <CANISTER>  Specifies the name of the canister

this feature allows developers to easily produce a url for a canister for a given network. The logic works as follows:

frontend canisters will use the ?canisterId query parameter for local urls
frontend canisters will point to .icp0.io for mainnet
backend canisters will link to the Candid UI frontend for the specified network, with the interface for the canister

This logic is largely provided in dfx-core, and the formatting is shared for both dfx deploy and dfx canister url output

How Has This Been Tested?

Newly added unit and e2e tests cover this feature

Checklist:

The title of this PR complies with Conventional Commits.
I have edited the CHANGELOG accordingly.
I have made corresponding changes to the documentation.

ericswanson-dfinity

Please consider reverting all of the changes to construct_frontend_url (the ic0.app renaming, the localhost special-casing) and construct_ui_canister_url. Instead, open two PRs:

This PR, which:
- refactors the frontend URL formatting methods by moving them as-is from dfx deploy to dfx-core
- adds dfx canister url which also calls them
A new PR, which changes the behavior of the frontend URL formatting methods

Including both changes together in this PR makes them harder to review for two reasons:

moving a method and changing it in the same PR looks like "deleted method A" and " added method B". This makes it more difficult to look at only what changed.
Conceptually they are two different changes: "added dfx canister url command" and "improved canister URLs [in these ways]". Submitting them separately will make it easier to focus on the changes.

Please update https://github.com/dfinity/sdk/blob/master/docs/cli-reference/dfx-canister.md

ericswanson-dfinity · 2023-09-01T20:01:05Z

e2e/tests-dfx/canister_url.bash

+    standard_teardown
+}
+
+@test "canister url performs as expected on local deploy" {


Could we have a test that demonstrates the expected behavior with a non-remote, non-pull canister on --network ic? The dfx deploy tests couldn't do this because they can't deploy to mainnet. The test would have to populate canister_ids.json.

ericswanson-dfinity · 2023-09-01T20:03:06Z

e2e/tests-dfx/canister_url.bash

+    assert_eq "http://127.0.0.1:4943/?canisterId=bd3sg-teaaa-aaaaa-qaaba-cai"
+}
+
+@test "canister url performs as expected on remote canisters" {


pullable canisters are different from remote canisters. Could we have a test for remote canisters too? They should show different urls depending on network local/ic, but don't need to deploy to mainnet in order to display a mainnet url.

ericswanson-dfinity · 2023-09-01T20:06:44Z

CHANGELOG.md

@@ -4,6 +4,12 @@

 ### fix: dfx deploy urls printed for asset canisters

+### feat: new subcommand: dfx canister url
+
+Dfx Canister URL is a new subcommand that allows you to print the URL of a canister on a specific network. Currently works for local and ic networks.


Suggested change

Dfx Canister URL is a new subcommand that allows you to print the URL of a canister on a specific network. Currently works for local and ic networks.

`dfx canister url` is a new subcommand that allows you to print the URL of a canister on a specific network. Currently works for local and ic networks.

ericswanson-dfinity · 2023-09-01T20:19:03Z

src/dfx-core/src/canister/url.rs

+
+const MAINNET_CANDID_INTERFACE_PRINCIPAL: &str = "a4gq6-oaaaa-aaaab-qaa4q-cai";
+
+pub fn format_frontend_url(provider: &Url, canister_id: &str) -> Url {


Please retain types in the method signature and convert to &str in the method

Suggested change

pub fn format_frontend_url(provider: &Url, canister_id: &str) -> Url {

pub fn format_frontend_url(provider: &Url, canister_id: &Principal) -> Url {

ericswanson-dfinity · 2023-09-01T20:19:12Z

src/dfx-core/src/canister/url.rs

+    url
+}
+
+pub fn format_ui_canister_url_ic(canister_id: &str) -> Result<Url, ParseError> {


Suggested change

pub fn format_ui_canister_url_ic(canister_id: &str) -> Result<Url, ParseError> {

pub fn format_ui_canister_url_ic(canister_id: &Principal) -> Result<Url, ParseError> {

ericswanson-dfinity · 2023-09-01T20:55:48Z

src/dfx/src/commands/canister/url.rs

+    if let Some(canisters) = &config.get_config().canisters {
+        let canister_config = canisters.get(canister_name).unwrap();


Never Nesting is more of an aspiration, but we can flatten this a bit, put the error messages next to the error conditions, and eliminate an unwrap:

Suggested change

if let Some(canisters) = &config.get_config().canisters {

let canister_config = canisters.get(canister_name).unwrap();

let canisters = config

.get_config()

.canisters

.as_ref()

.ok_or_else(|| anyhow::anyhow!("No canisters defined in dfx.json"))?;

let canister_config = canisters

.get(canister_name)

.ok_or_else(|| anyhow::anyhow!("Canister {} not found in dfx.json", canister_name))?;

ericswanson-dfinity · 2023-09-01T21:09:16Z

src/dfx/src/commands/deploy.rs

-                    let url = construct_ui_canister_url(network, &canister_id, ui_canister_id)?;
-                    if let Some(ui_canister_url) = url {
-                        candid_urls.insert(canister_name, ui_canister_url);
+                    let is_local = env.get_network_descriptor().name == "local";


What's the rationale in changing this logic? This used to check network.is_ic, which matches the condition for creating the candid UI canister.

Also the name of format_ui_canister_url_ic is a hint: "do this on IC", not "do this unless the network name is 'local'"

ericswanson-dfinity · 2023-09-01T21:11:47Z

src/dfx/src/commands/deploy.rs

+                    if is_local {
+                        let ui_canister_id = ui_canister_id.ok_or_else(|| {
+                            anyhow!(
+                                "The ui canister id is not set in the canister_id_store.json file."
+                            )
+                        })?;
+                        let url = format_ui_canister_url_custom(
+                            &&canister_id.to_string(),
+                            &provider,
+                            &ui_canister_id.to_string(),
+                        );
+                        candid_urls.insert(canister_name, url);
+                    } else {
+                        let url = format_ui_canister_url_ic(&canister_id.to_string())?;
+                        candid_urls.insert(canister_name, url);
                    }


Please DRY

Suggested change

if is_local {

let ui_canister_id = ui_canister_id.ok_or_else(|| {

anyhow!(

"The ui canister id is not set in the canister_id_store.json file."

)

})?;

let url = format_ui_canister_url_custom(

&&canister_id.to_string(),

&provider,

&ui_canister_id.to_string(),

);

candid_urls.insert(canister_name, url);

} else {

let url = format_ui_canister_url_ic(&canister_id.to_string())?;

candid_urls.insert(canister_name, url);

}

let url = if is_local {

let ui_canister_id = ui_canister_id.ok_or_else(|| {

anyhow!(

"The ui canister id is not set in the canister_id_store.json file."

)

})?;

format_ui_canister_url_custom(

&&canister_id.to_string(),

&provider,

&ui_canister_id.to_string(),

)

} else {

format_ui_canister_url_ic(&canister_id.to_string())?

};

candid_urls.insert(canister_name, url);

ericswanson-dfinity · 2023-09-01T21:14:38Z

src/dfx-core/src/canister/url.rs

+pub fn format_frontend_url(provider: &Url, canister_id: &str) -> Url {
+    let mut url = Url::clone(&provider);
+    if let Some(Domain(domain)) = url.host() {
+        if domain.ends_with("icp-api.io") || domain.ends_with("ic0.app") {


Looking for substrings in domain names is always iffy, but I suspect this should be the following:

Suggested change

if domain.ends_with("icp-api.io") || domain.ends_with("ic0.app") {

if domain == "icp-api.io" || domain.ends_with(".icp-api.io") || domain == "ic0.app" || domain.ends_with(".ic0.app") {

ericswanson-dfinity · 2023-09-01T21:16:49Z

src/dfx-core/src/canister/url.rs

+            let new_domain = domain.replace("icp-api.io", "icp0.io");
+            let new_domain = new_domain.replace("ic0.app", "icp0.io");


Hostname manipulation is error-prone. What if the domain is xyz-ic0.app-aaa.icp0.io?

ericswanson-dfinity · 2023-09-01T21:32:20Z

src/dfx-core/src/canister/url.rs

+            let new_domain = new_domain.replace("ic0.app", "icp0.io");
+            let host = format!("{}.{}", canister_id, new_domain);
+            let _ = url.set_host(Some(&host));
+        } else if domain.contains("localhost") {


What if it's not-a-localhost.my-domain.yay

krpeacock added 6 commits August 30, 2023 14:25

feat: adds dfx canister url subcommand

2dfb6b9

feat: dfx_core url functions and new command

21e36ac

unit tests and refactoring deploy

8024c14

e2e tests

1e204de

more e2e tests

d48ead5

cargo fmt

76fea27

krpeacock requested review from chenyan-dfinity and a team as code owners September 1, 2023 19:48

ericswanson-dfinity requested changes Sep 1, 2023

View reviewed changes

ericswanson-dfinity reviewed Sep 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dfx canister url #3346

feat: dfx canister url #3346

krpeacock commented Sep 1, 2023 •

edited

Loading

ericswanson-dfinity left a comment

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

ericswanson-dfinity Sep 1, 2023

	Dfx Canister URL is a new subcommand that allows you to print the URL of a canister on a specific network. Currently works for local and ic networks.
	`dfx canister url` is a new subcommand that allows you to print the URL of a canister on a specific network. Currently works for local and ic networks.


		const MAINNET_CANDID_INTERFACE_PRINCIPAL: &str = "a4gq6-oaaaa-aaaab-qaa4q-cai";

		pub fn format_frontend_url(provider: &Url, canister_id: &str) -> Url {

	pub fn format_ui_canister_url_ic(canister_id: &str) -> Result<Url, ParseError> {
	pub fn format_ui_canister_url_ic(canister_id: &Principal) -> Result<Url, ParseError> {

		if let Some(canisters) = &config.get_config().canisters {
		let canister_config = canisters.get(canister_name).unwrap();

	if domain.ends_with("icp-api.io") \|\| domain.ends_with("ic0.app") {
	if domain == "icp-api.io" \|\| domain.ends_with(".icp-api.io") \|\| domain == "ic0.app" \|\| domain.ends_with(".ic0.app") {

		let new_domain = domain.replace("icp-api.io", "icp0.io");
		let new_domain = new_domain.replace("ic0.app", "icp0.io");

feat: dfx canister url #3346

Are you sure you want to change the base?

feat: dfx canister url #3346

Conversation

krpeacock commented Sep 1, 2023 • edited Loading

Description

How Has This Been Tested?

Checklist:

ericswanson-dfinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krpeacock commented Sep 1, 2023 •

edited

Loading