-
Notifications
You must be signed in to change notification settings - Fork 85
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
Conversation
@@ -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> { |
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.
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)
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.
We could lose the A
type parameter in doing that since each *Response
enum could specify the type of its reply
.
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.
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.
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.
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), |
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.
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.
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.
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?
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.
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
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.
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.
It can install a canister for now.
f8db79b
to
0f2881e
Compare
1f5b0da
to
35192a3
Compare
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.
I think a test called install_code_request_serialization
that is equivalent to query_request_serialization
would be good.
Added 3 tests - Serialization, Response 200, Response 400. |
@paulyoung Please take another look. |
temp_dir() returns the root temp directory and not a new one.
692d879
to
d616773
Compare
Jira: DFX-429
When using the output of a
dfx build
on an ActorScript file, we getThis is from the decoding of the response. I'll fix that before moving this to an official PR.