-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: dfx canister url #3346
base: master
Are you sure you want to change the base?
feat: dfx canister url #3346
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- refactors the frontend URL formatting methods by moving them as-is from
- 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
standard_teardown | ||
} | ||
|
||
@test "canister url performs as expected on local deploy" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
assert_eq "http://127.0.0.1:4943/?canisterId=bd3sg-teaaa-aaaaa-qaaba-cai" | ||
} | ||
|
||
@test "canister url performs as expected on remote canisters" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please retain types in the method signature and convert to &str in the method
pub fn format_frontend_url(provider: &Url, canister_id: &str) -> Url { | |
pub fn format_frontend_url(provider: &Url, canister_id: &Principal) -> Url { |
url | ||
} | ||
|
||
pub fn format_ui_canister_url_ic(canister_id: &str) -> Result<Url, ParseError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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))?; |
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'"
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please DRY
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); |
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for substrings in domain names is always iffy, but I suspect this should be the following:
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hostname manipulation is error-prone. What if the domain is xyz-ic0.app-aaa.icp0.io
?
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it's not-a-localhost.my-domain.yay
Description
Adds a new canister subcommand -
dfx canister url
this feature allows developers to easily produce a url for a canister for a given network. The logic works as follows:
This logic is largely provided in
dfx-core
, and the formatting is shared for bothdfx deploy
anddfx canister url
outputHow Has This Been Tested?
Newly added unit and e2e tests cover this feature
Checklist: