Skip to content
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

New Feature: dfx canister install #36

Merged
merged 6 commits into from Sep 18, 2019
Merged

New Feature: dfx canister install #36

merged 6 commits into from Sep 18, 2019

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Sep 13, 2019

Jira: DFX-429

When using the output of a dfx build on an ActorScript file, we get

An error occured:
SerdeCbor(
    ErrorImpl {
        code: EofWhileParsingValue,
        offset: 0,
    },
)

This is from the decoding of the response. I'll fix that before moving this to an official PR.

@@ -55,13 +55,13 @@ enum Request {
#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
#[serde(tag = "status")]
pub enum Response<A> {
Accepted,
pub enum ReadResponse<A> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have RequestStatusResponse and QueryResponse since the statuses for request-status and query are different now (maybe they always were and I implemented this incorrectly)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could lose the A type parameter in doing that since each *Response enum could specify the type of its reply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering how far I should take this PR in refactoring the old stuff. I guess I'll have to work on this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'd like to steer away from changing the old stuff. Let's stay focused.

I created a task in JIRA to refactor the API client. https://dfinity.atlassian.net/browse/SDK-446

.arg(
Arg::with_name("wasm")
.help("The wasm file to use.")
.required(true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are assigning names to canisters in the project config, can we just provide the name of the canister we want to install instead?

The about text implies that a canister will be built so I'm a bit confused why we'd need to provide a wasm file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the flow breaks a bit. Right now we only have a canister 42. create_canister will return a "random" ID so we might not want to store it in the JSON (since restarting the net from nothing would give non-deterministic IDs). The best location would probably be to have our own coordinator on the test net that will be a store of name to canister ID and do a query before we do every submit. That's the best I could come up with.

Do you have any ideas to improve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the ID and wasm. I added a task for implementing the env.production.json mapping we discussed: https://dfinity.atlassian.net/browse/SDK-447

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any ideas to improve this?

Based on the config file format (the last time I looked) I was thinking that something like dfx install hello would amount to looking at the canisters.hello.main field, building that, and then installing the resulting Wasm file.

dfx/src/commands/canister/install.rs Outdated Show resolved Hide resolved
dfx/src/commands/canister/install.rs Show resolved Hide resolved
dfx/src/commands/canister/install.rs Show resolved Hide resolved
dfx/src/lib/api_client.rs Outdated Show resolved Hide resolved
dfx/src/lib/api_client.rs Outdated Show resolved Hide resolved
dfx/src/lib/api_client.rs Outdated Show resolved Hide resolved
dfx/src/lib/api_client.rs Outdated Show resolved Hide resolved
@hansl hansl force-pushed the hansl/canister-install branch 2 times, most recently from f8db79b to 0f2881e Compare September 18, 2019 04:01
@hansl hansl marked this pull request as ready for review September 18, 2019 04:05
dfx/src/lib/env.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@paulyoung paulyoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a test called install_code_request_serialization that is equivalent to query_request_serialization would be good.

@hansl
Copy link
Contributor Author

hansl commented Sep 18, 2019

Added 3 tests - Serialization, Response 200, Response 400.

@hansl
Copy link
Contributor Author

hansl commented Sep 18, 2019

@paulyoung Please take another look.

@hansl hansl merged commit 4ea44e7 into master Sep 18, 2019
@hansl hansl deleted the hansl/canister-install branch September 20, 2019 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants