Revert "refactor: always fetch did file from canister when making canister calls (#2931)" (#3555)

ericswanson-dfinity · web-flow · commit 904c44001f98 · 2024-01-30T17:15:52.000-08:00
This reverts commit 935ffc7.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,10 +12,6 @@ When using `dfx canister status`, the output now includes the new query statisti
 
 Fix the bug that when parsing `vec {1;2;3}` with `blob` type, dfx silently ignores the numbers.
 
-### fix!: always fetch did file from canister when making canister calls
-
-`dfx canister call` will always fetch did file from the canister metadata. This is especially helpful for calling remote canisters. It's a breaking change in the sense that if the canister doesn't have the `candid:service` metadata, we will not read the local did file from build artifact, and dfx will issue a warning in this case to encourage canister developers to put the did file into canister metadata.
-
 ### fix: support `import` for local did file
 
 If the local did file contains `import` or init args, dfx will rewrite the did file when storing in canister metadata.
diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash
@@ -266,7 +266,7 @@ check_permission_failure() {
   FE_CANISTER_ID="$(dfx canister id e2e_project_frontend)"
   rm .dfx/local/canister_ids.json
   assert_command_fail dfx canister call "$FE_CANISTER_ID" validate_revoke_permission "(record { of_principal=principal \"$PREPARE_PRINCIPAL\"; permission = variant { FlyBeFree }; })"
-  assert_contains "FlyBeFree not found"
+  assert_contains "trapped"
 }
 
 @test "access control - fine-grained" {
@@ -905,29 +905,24 @@ check_permission_failure() {
   dfx canister install e2e_project_frontend
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("/binary/noise.txt")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "\b8\01\20\80\0a\77\31\32\20\00\78\79\0a\4b\4c\0b\0a\6a\6b")' "$stdout"
+  assert_eq '(blob "\b8\01\20\80\0a\77\31\32\20\00\78\79\0a\4b\4c\0b\0a\6a\6b")'
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("/text-with-newlines.txt")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "cherries\0ait\27s cherry season\0aCHERRIES")' "$stdout"
+  assert_eq '(blob "cherries\0ait\27s cherry season\0aCHERRIES")'
 
   assert_command dfx canister call --update e2e_project_frontend store '(record{key="AA"; content_type="text/plain"; content_encoding="identity"; content=blob "hello, world!"})'
   assert_eq '()'
   assert_command dfx canister call --update e2e_project_frontend store '(record{key="B"; content_type="application/octet-stream"; content_encoding="identity"; content=blob"XWV"})'
   assert_eq '()'
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("B")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "XWV")' "$stdout"
+  assert_eq '(blob "XWV")'
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("AA")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "hello, world!")' "$stdout"
+  assert_eq '(blob "hello, world!")'
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("B")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "XWV")' "$stdout"
+  assert_eq '(blob "XWV")'
 
   assert_command_fail dfx canister call --query e2e_project_frontend retrieve '("C")'
 }
diff --git a/e2e/tests-dfx/build_granular.bash b/e2e/tests-dfx/build_granular.bash
@@ -67,12 +67,10 @@ teardown() {
   dfx canister install e2e_project_frontend
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("/binary/noise.txt")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "\b8\01\20\80\0a\77\31\32\20\00\78\79\0a\4b\4c\0b\0a\6a\6b")' "$stdout"
+  assert_eq '(blob "\b8\01\20\80\0a\77\31\32\20\00\78\79\0a\4b\4c\0b\0a\6a\6b")'
 
   assert_command dfx canister call --query e2e_project_frontend retrieve '("/text-with-newlines.txt")' --output idl
-  # shellcheck disable=SC2154
-  assert_eq '(blob "cherries\0ait\27s cherry season\0aCHERRIES")' "$stdout"
+  assert_eq '(blob "cherries\0ait\27s cherry season\0aCHERRIES")'
 }
 
 @test "cyclic dependencies are detected" {
diff --git a/e2e/tests-dfx/call.bash b/e2e/tests-dfx/call.bash
@@ -26,9 +26,9 @@ teardown() {
   CANISTER_ID=$(dfx canister id hello_backend)
   rm .dfx/local/canister_ids.json
 
-  # given a canister id, fetch the did file from metadata
+  # if no candid file known, then no field names
   assert_command dfx canister call "$CANISTER_ID" make_struct '("A", "B")'
-  assert_eq '(record { c = "A"; d = "B" })'
+  assert_eq '(record { 99 = "A"; 100 = "B" })'
 
   # if passing the candid file, field names available
   assert_command dfx canister call --candid .dfx/local/canisters/hello_backend/hello_backend.did "$CANISTER_ID" make_struct '("A", "B")'
diff --git a/e2e/tests-dfx/identity.bash b/e2e/tests-dfx/identity.bash
@@ -155,8 +155,7 @@ teardown() {
   assert_command dfx canister call e2e_project_frontend store '(record{key="B"; content_type="application/octet-stream"; content_encoding="identity"; content=blob"XWV"})' --identity alice
   assert_eq '()'
   assert_command dfx canister call --output idl e2e_project_frontend retrieve '("B")'
-  # shellcheck disable=SC2154
-  assert_eq '(blob "XWV")' "$stdout"
+  assert_eq '(blob "XWV")'
 }
 
 @test "after renaming an identity, the renamed identity is still initializer" {
@@ -182,8 +181,7 @@ teardown() {
   assert_command dfx canister call e2e_project_frontend store '(record{key="B"; content_type="application/octet-stream"; content_encoding="identity"; content=blob "hello"})' --identity bob
   assert_eq '()'
   assert_command dfx canister call --output idl e2e_project_frontend retrieve '("B")'
-    # shellcheck disable=SC2154
-  assert_eq '(blob "hello")' "$stdout"
+  assert_eq '(blob "hello")'
 }
 
 @test "using an unencrypted identity on mainnet provokes a warning" {
diff --git a/e2e/tests-dfx/wallet.bash b/e2e/tests-dfx/wallet.bash
@@ -153,8 +153,7 @@ teardown() {
 
   assert_command dfx canister call "$WALLET" wallet_call \
     "(record { canister = principal \"$(dfx canister id e2e_project_backend)\"; method_name = \"amInitializer\"; args = blob \"DIDL\00\00\"; cycles = (0:nat64)})"
-  # shellcheck disable=SC2154
-  assert_eq '(variant { 17_724 = record { 153_986_224 = blob "\44\49\44\4c\00\01\7e\01" } })' "$stdout"  # True in DIDL.
+  assert_eq '(variant { 17_724 = record { 153_986_224 = blob "\44\49\44\4c\00\01\7e\01" } })'  # True in DIDL.
 }
 
 @test "forward user call through wallet: deploy" {
@@ -170,8 +169,7 @@ teardown() {
   assert_command dfx canister call e2e_project_backend amInitializer
   assert_command dfx canister call "$WALLET" wallet_call \
     "(record { canister = principal \"$(dfx canister id e2e_project_backend)\"; method_name = \"amInitializer\"; args = blob \"DIDL\00\00\"; cycles = (0:nat64)})"
-  # shellcheck disable=SC2154
-  assert_eq '(variant { 17_724 = record { 153_986_224 = blob "\44\49\44\4c\00\01\7e\01" } })' "$stdout"  # True in DIDL.
+  assert_eq '(variant { 17_724 = record { 153_986_224 = blob "\44\49\44\4c\00\01\7e\01" } })'  # True in DIDL.
 }
 
 @test "a 64-bit wallet can still be called in the 128-bit context" {
diff --git a/src/dfx/src/commands/canister/call.rs b/src/dfx/src/commands/canister/call.rs
@@ -4,13 +4,10 @@ use crate::lib::error::DfxResult;
 use crate::lib::operations::canister::get_local_cid_and_candid_path;
 use crate::lib::root_key::fetch_root_key_if_needed;
 use crate::util::clap::parsers::{cycle_amount_parser, file_or_stdin_parser};
-use crate::util::{
-    arguments_from_file, blob_from_arguments, get_candid_type, print_idl_blob, read_module_metadata,
-};
+use crate::util::{arguments_from_file, blob_from_arguments, get_candid_type, print_idl_blob};
 use anyhow::{anyhow, Context};
 use candid::Principal as CanisterId;
 use candid::{CandidType, Decode, Deserialize, Principal};
-use candid_parser::utils::CandidSource;
 use clap::Parser;
 use dfx_core::canister::build_wallet_canister;
 use dfx_core::identity::CallSender;
@@ -221,32 +218,29 @@ pub async fn exec(
     opts: CanisterCallOpts,
     call_sender: &CallSender,
 ) -> DfxResult {
-    let agent = env.get_agent();
-    fetch_root_key_if_needed(env).await?;
-
     let callee_canister = opts.canister_name.as_str();
     let method_name = opts.method_name.as_str();
     let canister_id_store = env.get_canister_id_store()?;
 
-    let canister_id = match CanisterId::from_text(callee_canister) {
-        Ok(id) => id,
+    let (canister_id, maybe_candid_path) = match CanisterId::from_text(callee_canister) {
+        Ok(id) => {
+            if let Some(canister_name) = canister_id_store.get_name(callee_canister) {
+                get_local_cid_and_candid_path(env, canister_name, Some(id))?
+            } else {
+                // TODO fetch candid file from remote canister
+                (id, None)
+            }
+        }
         Err(_) => {
             let canister_id = canister_id_store.get(callee_canister)?;
-            get_local_cid_and_candid_path(env, callee_canister, Some(canister_id))?.0
+            get_local_cid_and_candid_path(env, callee_canister, Some(canister_id))?
         }
     };
-    let method_type = if let Some(path) = opts.candid {
-        get_candid_type(CandidSource::File(&path), method_name)
-    } else {
-        read_module_metadata(agent, canister_id, "candid:service")
-            .await
-            .and_then(|did| get_candid_type(CandidSource::Text(&did), method_name))
-    };
-    if method_type.is_none() {
-        eprintln!("Cannot fetch Candid interface from canister metadata, sending arguments with inferred types.");
-    }
+    let maybe_candid_path = opts.candid.or(maybe_candid_path);
 
     let is_management_canister = canister_id == CanisterId::management_canister();
+
+    let method_type = maybe_candid_path.and_then(|path| get_candid_type(&path, method_name));
     let is_query_method = method_type.as_ref().map(|(_, f)| f.is_query());
 
     let arguments_from_file = opts
@@ -281,6 +275,9 @@ pub async fn exec(
     // Get the argument, get the type, convert the argument to the type and return
     // an error if any of it doesn't work.
     let arg_value = blob_from_arguments(arguments, opts.random.as_deref(), arg_type, &method_type)?;
+    let agent = env.get_agent();
+
+    fetch_root_key_if_needed(env).await?;
 
     // amount has been validated by cycle_amount_validator
     let cycles = opts.with_cycles.unwrap_or(0);
diff --git a/src/dfx/src/commands/canister/sign.rs b/src/dfx/src/commands/canister/sign.rs
@@ -8,7 +8,6 @@ use crate::util::clap::parsers::file_or_stdin_parser;
 use crate::util::{arguments_from_file, blob_from_arguments, get_candid_type};
 use anyhow::{anyhow, bail, Context};
 use candid::Principal;
-use candid_parser::utils::CandidSource;
 use clap::Parser;
 use dfx_core::identity::CallSender;
 use ic_agent::AgentError;
@@ -87,7 +86,7 @@ pub async fn exec(
             if let Some(canister_name) = canister_id_store.get_name(callee_canister) {
                 get_local_cid_and_candid_path(env, canister_name, Some(id))?
             } else {
-                // Sign works in offline mode, cannot fetch candid file from remote canister
+                // TODO fetch candid file from remote canister
                 (id, None)
             }
         }
@@ -97,8 +96,7 @@ pub async fn exec(
         }
     };
 
-    let method_type =
-        maybe_candid_path.and_then(|path| get_candid_type(CandidSource::File(&path), method_name));
+    let method_type = maybe_candid_path.and_then(|path| get_candid_type(&path, method_name));
     let is_query_method = method_type.as_ref().map(|(_, f)| f.is_query());
 
     let arguments_from_file = opts
diff --git a/src/dfx/src/commands/deps/init.rs b/src/dfx/src/commands/deps/init.rs
@@ -5,10 +5,10 @@ use crate::lib::deps::{
 };
 use crate::lib::environment::Environment;
 use crate::lib::error::DfxResult;
-use crate::util::fuzzy_parse_argument;
+use crate::util::{check_candid_file, fuzzy_parse_argument};
 use anyhow::{anyhow, bail};
 use candid::Principal;
-use candid_parser::{types::IDLTypes, typing::ast_to_type, utils::CandidSource};
+use candid_parser::{types::IDLTypes, typing::ast_to_type};
 use clap::Parser;
 use slog::{info, warn, Logger};
 
@@ -68,7 +68,7 @@ fn set_init(
         .ok_or_else(|| anyhow!("Failed to find {canister_id} entry in pulled.json"))?;
     let canister_prompt = get_canister_prompt(canister_id, pulled_canister);
     let idl_path = get_pulled_service_candid_path(canister_id)?;
-    let (env, _) = CandidSource::File(&idl_path).load()?;
+    let (env, _) = check_candid_file(&idl_path)?;
     let candid_args = pulled_json.get_candid_args(canister_id)?;
     let candid_args_idl_types: IDLTypes = candid_args.parse()?;
     let mut types = vec![];
diff --git a/src/dfx/src/commands/remote/generate_binding.rs b/src/dfx/src/commands/remote/generate_binding.rs
@@ -2,8 +2,8 @@ use crate::lib::agent::create_agent_environment;
 use crate::lib::environment::Environment;
 use crate::lib::error::DfxResult;
 use crate::lib::models::canister::CanisterPool;
+use crate::util::check_candid_file;
 use anyhow::Context;
-use candid_parser::utils::CandidSource;
 use clap::Parser;
 use slog::info;
 
@@ -70,7 +70,7 @@ pub fn exec(env: &dyn Environment, opts: GenerateBindingOpts) -> DfxResult {
                         continue;
                     }
                 }
-                let (type_env, did_types) = CandidSource::File(&candid).load()?;
+                let (type_env, did_types) = check_candid_file(&candid)?;
                 let extension = main.extension().unwrap_or_default();
                 let bindings = if extension == "mo" {
                     Some(candid_parser::bindings::motoko::compile(
diff --git a/src/dfx/src/lib/builders/mod.rs b/src/dfx/src/lib/builders/mod.rs
@@ -3,9 +3,9 @@ use crate::lib::canister_info::CanisterInfo;
 use crate::lib::environment::Environment;
 use crate::lib::error::{BuildError, DfxError, DfxResult};
 use crate::lib::models::canister::CanisterPool;
+use crate::util::check_candid_file;
 use anyhow::{anyhow, bail, Context};
 use candid::Principal as CanisterId;
-use candid_parser::utils::CandidSource;
 use dfx_core::config::model::dfinity::{Config, Profile};
 use dfx_core::network::provider::get_network_context;
 use dfx_core::util;
@@ -150,7 +150,7 @@ pub trait CanisterBuilder {
 
         let generated_idl_path = self.generate_idl(pool, info, config)?;
 
-        let (env, ty) = CandidSource::File(generated_idl_path.as_path()).load()?;
+        let (env, ty) = check_candid_file(generated_idl_path.as_path())?;
 
         // Typescript
         if bindings.contains(&"ts".to_string()) {
diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs
@@ -8,10 +8,9 @@ use crate::lib::error::{BuildError, DfxError, DfxResult};
 use crate::lib::metadata::dfx::DfxMetadata;
 use crate::lib::metadata::names::{CANDID_ARGS, CANDID_SERVICE, DFX};
 use crate::lib::wasm::file::{compress_bytes, read_wasm_module};
-use crate::util::assets;
+use crate::util::{assets, check_candid_file};
 use anyhow::{anyhow, bail, Context};
 use candid::Principal as CanisterId;
-use candid_parser::utils::CandidSource;
 use dfx_core::config::model::canister_id_store::CanisterIdStore;
 use dfx_core::config::model::dfinity::{
     CanisterMetadataSection, Config, MetadataVisibility, WasmOptLevel,
@@ -276,15 +275,15 @@ impl Canister {
 
         let IdlBuildOutput::File(build_idl_path) = &build_output.idl;
 
-        // 1. Separate into constructor.did, service.did and init_args
-        let (constructor_did, service_did, init_args) = separate_candid(build_idl_path)?;
-
-        // 2. Copy the constructor IDL file to .dfx/local/canisters/NAME/constructor.did.
+        // 1. Copy the complete IDL file to .dfx/local/canisters/NAME/constructor.did.
         let constructor_idl_path = self.info.get_constructor_idl_path();
         dfx_core::fs::composite::ensure_parent_dir_exists(&constructor_idl_path)?;
-        dfx_core::fs::write(&constructor_idl_path, constructor_did)?;
+        dfx_core::fs::copy(build_idl_path, &constructor_idl_path)?;
         dfx_core::fs::set_permissions_readwrite(&constructor_idl_path)?;
 
+        // 2. Separate into service.did and init_args
+        let (service_did, init_args) = separate_candid(build_idl_path)?;
+
         // 3. Save service.did into following places in .dfx/local/:
         //   - canisters/NAME/service.did
         //   - IDL_ROOT/CANISTER_ID.did
@@ -338,7 +337,7 @@ fn wasm_opt_level_convert(opt_level: WasmOptLevel) -> OptLevel {
     }
 }
 
-fn separate_candid(path: &Path) -> DfxResult<(String, String, String)> {
+fn separate_candid(path: &Path) -> DfxResult<(String, String)> {
     use candid::pretty::candid::{compile, pp_args};
     use candid::types::internal::TypeInner;
     use candid_parser::{
@@ -351,37 +350,34 @@ fn separate_candid(path: &Path) -> DfxResult<(String, String, String)> {
         .decs
         .iter()
         .any(|dec| matches!(dec, Dec::ImportType(_) | Dec::ImportServ(_)));
-    let (env, actor) = CandidSource::File(path).load()?;
+    let (env, actor) = check_candid_file(path)?;
     let actor = actor.ok_or_else(|| anyhow!("provided candid file contains no main service"))?;
     let actor = env.trace_type(&actor)?;
     let has_init_args = matches!(actor.as_ref(), TypeInner::Class(_, _));
     if has_imports || has_init_args {
         let (init_args, serv) = match actor.as_ref() {
             TypeInner::Class(args, ty) => (args.clone(), ty.clone()),
-            TypeInner::Service(_) => (vec![], actor.clone()),
+            TypeInner::Service(_) => (vec![], actor),
             _ => unreachable!(),
         };
         let init_args = pp_args(&init_args).pretty(80).to_string();
         let service = compile(&env, &Some(serv));
-        let constructor = compile(&env, &Some(actor));
-        Ok((constructor, service, init_args))
+        Ok((service, init_args))
     } else {
         // Keep the original did file to preserve comments
-        Ok((did.clone(), did, "()".to_string()))
+        Ok((did, "()".to_string()))
     }
 }
 
 #[context("{} is not a valid subtype of {}", specified_idl_path.display(), compiled_idl_path.display())]
 fn check_valid_subtype(compiled_idl_path: &Path, specified_idl_path: &Path) -> DfxResult {
     use candid::types::subtype::{subtype_with_config, OptReport};
-    let (mut env, opt_specified) = CandidSource::File(specified_idl_path)
-        .load()
-        .context("Checking specified candid file.")?;
+    let (mut env, opt_specified) =
+        check_candid_file(specified_idl_path).context("Checking specified candid file.")?;
     let specified_type =
         opt_specified.expect("Specified did file should contain some service interface");
-    let (env2, opt_compiled) = CandidSource::File(compiled_idl_path)
-        .load()
-        .context("Checking compiled candid file.")?;
+    let (env2, opt_compiled) =
+        check_candid_file(compiled_idl_path).context("Checking compiled candid file.")?;
     let compiled_type =
         opt_compiled.expect("Compiled did file should contain some service interface");
     let mut gamma = HashSet::new();
@@ -832,7 +828,7 @@ fn build_canister_js(canister_id: &CanisterId, canister_info: &CanisterInfo) ->
         .get_service_idl_path()
         .with_extension("did.d.ts");
 
-    let (env, ty) = CandidSource::File(&canister_info.get_constructor_idl_path()).load()?;
+    let (env, ty) = check_candid_file(&canister_info.get_service_idl_path())?;
     let content = ensure_trailing_newline(candid_parser::bindings::javascript::compile(&env, &ty));
     std::fs::write(&output_did_js_path, content).with_context(|| {
         format!(
diff --git a/src/dfx/src/lib/operations/canister/install_canister.rs b/src/dfx/src/lib/operations/canister/install_canister.rs
diff --git a/src/dfx/src/util/mod.rs b/src/dfx/src/util/mod.rs
