feat: replace manifest.json with {.dfx/}canister_ids.json #815

ericswanson-dfinity · 2020-07-14T23:42:46Z

Implements #780

Removed the canister manifest (which had one canister ID per canister, as well as paths to outputs) and replaced it with two files:

.dfx/canister_ids.json: stores ephemeral canister ids for (typically) local "networks"
canister_ids.json: stores persistent canister ids for (typically) remote networks.

The dfx create command populates these files, while other commands use the values.

Added a NetworkDescriptor field to Environment (though really only AgentEnvironment), in order to tell which canister_ids.json file to use.

Added to dfx.json networks a type field ("persistent" or "ephemeral")

p-shahi · 2020-07-15T00:38:44Z

@ericswanson-dfinity I believe src/dfx/assets/new_project_files/dfx.json should be updated with the new json fields...although, the e2e tests seem to indicate that is not necessary!

chenyan-dfinity · 2020-07-15T00:45:21Z

src/dfx/src/lib/locations.rs

+use std::path::PathBuf;
+
+pub fn canister_did_location(canister_name: &str) -> PathBuf {
+    PathBuf::from(format!("canisters/{}/{}.did", canister_name, canister_name))


Just to double check: is the did file always under canisters/ or the build output directory specified in dfx.json?

I thought we were going to remove that output directory (and have everything go under .dfx/somewhere), but until that is done, this should probably point to wherever dfx.json says.

hansl

Some changes, but nothing major.

hansl · 2020-07-15T01:36:56Z

src/dfx/src/lib/error/mod.rs

@@ -173,6 +185,25 @@ impl Display for DfxError {
            DfxError::NoLocalNetworkProviderFound => {
                f.write_str("Expected there to be a local network with a bind address")?;
            }
+            DfxError::CouldNotFindCanisterIdForNetwork(canister, network) => {
+                f.write_fmt(format_args!(
+                    "Cannot find canister id.  Please issue 'dfx canister --network {} create {}'.",


Can we special case network == "local" if that's the default?

Hah, I did first try to only make it include the --network {} part if network != "local", but ran into trouble. I'll try again.

src/dfx/src/lib/models/canister_id_store.rs

hansl · 2020-07-15T01:58:52Z

src/dfx/src/lib/canister_info.rs

-            canister_id: RefCell::new(None),
-
-            manifest_path,
+            canister_id: RefCell::new(canister_id),


Seems to me we don't need a RefCell anymore (since we panic if it's not set), correct? If so I'd like to get rid of it. They're usually an anti-pattern. Simply Option should do the trick.

hansl · 2020-07-15T02:00:38Z

src/dfx/src/lib/environment.rs

        let identity = get_profile_path().expect("Failed to access profile");
+        let agent_url = network_descriptor.providers.first().unwrap();


Should probably be random in this case. Actually, could you add an issue to have ic-agent support having multiple providers in round robin? This would probably be the better place to make this change. If you do that then this code is okay for now.

hansl · 2020-07-15T02:02:55Z

src/dfx/src/lib/canister_info.rs

        let canister_type = canister_config
            .r#type
            .as_ref()
            .cloned()
            .unwrap_or_else(|| "motoko".to_owned());

+        let canister_id = canister_id_store
+            .map(|store| store.get_canister_id(name))
+            .transpose()?;


Oh that's neat.

hansl · 2020-07-15T02:04:10Z

src/dfx/src/lib/models/canister.rs

+
+            let maybe_canister_id = canister_id_store.find_canister_id(&info.get_name());
+            if let Some(canister_id) = maybe_canister_id {
+                info.set_canister_id(canister_id)?;


This might be why it's still a RefCell, but it seems we could pass that to the constructor... or not sure actually.

hansl · 2020-07-15T02:06:22Z

src/dfx/src/lib/models/canister_id_store.rs

+    pub ids: CanisterIds,
+}
+
+impl CanisterIdStore {


Since many functions here has canister_id, what do you think of simplifying their names as it's redundant and the types already give you that information. So add_canister_id => add, find_canister_id => find. Also, remove canister where it feels redundant; get_canister_name => get_name.

Yes, sounds good

…for canister did location

Co-authored-by: Hans <[email protected]>

ericswanson-dfinity · 2020-07-15T17:30:30Z

src/dfx/src/lib/models/canister.rs

@@ -258,15 +214,6 @@ impl CanisterPool {
        for canister in &self.canisters {
            if build_config.build_mode_check {
                canister.generate_and_set_canister_id()?;


@hansl When trying to remove the RefCell, I run into this here:

error[E0596]: cannot borrow data in an `Arc` as mutable --> src/dfx/src/lib/models/canister.rs:218:17 | 218 | canister.generate_and_set_canister_id()?; | ^^^^^^^^ cannot borrow as mutable | = help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `std::sync::Arc<lib::models::canister::Canister>`

Yes, it makes canister info immutable. If that's too much trouble we can defer to another PR.

p-shahi · 2020-07-15T18:44:18Z

src/dfx/src/lib/models/canister.rs

-pub struct CanisterManifest {
-    pub canisters: Map<String, serde_json::value::Value>,
-}
-
 #[derive(Serialize, Deserialize, Debug)]
 pub struct CanManMetadata {


this struct should be deleted

…isterIdStore::get_for_env

feat: replace manifest.json with canister_ids.json

b45ad58

ericswanson-dfinity requested a review from a team as a code owner July 14, 2020 23:42

format

e8d8072

chenyan-dfinity reviewed Jul 15, 2020

View reviewed changes

hansl suggested changes Jul 15, 2020

View reviewed changes

ericswanson-dfinity and others added 2 commits July 14, 2020 19:27

add network type defaults to new dfx.json; use defaults.build.output …

f8c54ff

…for canister did location

Return Result directly

0de9910

Co-authored-by: Hans <[email protected]>

ericswanson-dfinity mentioned this pull request Jul 15, 2020

have agent-rs support having multiple providers in round robin #817

Closed

ericswanson-dfinity added 2 commits July 15, 2020 09:43

de-stutter CanisterIdStore interface

2fb5712

create does not need to load CanisterInfo, nor set canister id in it

6096e30

ericswanson-dfinity commented Jul 15, 2020

View reviewed changes

Only include --network parameter in error message for non-local network

aae012a

p-shahi reviewed Jul 15, 2020

View reviewed changes

ericswanson-dfinity added 3 commits July 15, 2020 12:00

canister errors may be reported in either order

92726e1

Change CanisterInfo::canister_id RefCell to Option

439113d

serde error propagation refinement, remove CanManMetadata, remove Can…

1f9d5d6

…isterIdStore::get_for_env

hansl approved these changes Jul 15, 2020

View reviewed changes

ericswanson-dfinity added the automerge-squash label Jul 15, 2020

mergify bot merged commit 88e2e41 into master Jul 15, 2020

mergify bot removed the automerge-squash label Jul 15, 2020

p-shahi deleted the ericswanson-780-canister-ids-json branch July 15, 2020 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace manifest.json with {.dfx/}canister_ids.json #815

feat: replace manifest.json with {.dfx/}canister_ids.json #815

ericswanson-dfinity commented Jul 14, 2020

p-shahi commented Jul 15, 2020 •

edited

Loading

chenyan-dfinity Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

hansl left a comment

hansl Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

hansl Jul 15, 2020

hansl Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

hansl Jul 15, 2020

hansl Jul 15, 2020

hansl Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

hansl Jul 15, 2020 •

edited

Loading

p-shahi Jul 15, 2020

ericswanson-dfinity Jul 15, 2020

		let identity = get_profile_path().expect("Failed to access profile");
		let agent_url = network_descriptor.providers.first().unwrap();

feat: replace manifest.json with {.dfx/}canister_ids.json #815

feat: replace manifest.json with {.dfx/}canister_ids.json #815

Conversation

ericswanson-dfinity commented Jul 14, 2020

p-shahi commented Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansl 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

hansl Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-shahi commented Jul 15, 2020 •

edited

Loading

hansl Jul 15, 2020 •

edited

Loading