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

Canister query and call commands #39

Merged
merged 5 commits into from
Sep 24, 2019
Merged

Canister query and call commands #39

merged 5 commits into from
Sep 24, 2019

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Sep 20, 2019

Call will generate a random nonce for now, to avoid errors.

There is currently no way to request status. As well, it seems that any changes to the memory at least in an actorscript build from the Counter example (see dfinity/motoko#660).

  • dfx canister query 42 greet DFINITY will work fine with the builtin canister.
  • dfx canister call 42 greet DFINITY works as well with the builtin canister (though can't read the result).
  • dfx canister install 42 PATH_TO_WASM works and updates the canister with the new counter wasm (or any wasm built).
  • Any query or call to any method (either read or inc) from the canister will kill the client, and restart it with the builtin canister.

I don't know if it's a problem with the actorscript output or the execution environment, but AFAICT the CLI makes the right call according to the public spec.

@hansl hansl force-pushed the hansl/canister-call branch 2 times, most recently from 5893e85 to 270b856 Compare September 20, 2019 20:14
dfx/src/commands/canister/call.rs Outdated Show resolved Hide resolved
dfx/src/commands/canister/call.rs Show resolved Hide resolved
client,
canister_id,
method_name.to_owned(),
arguments.map(|args| Blob(Vec::from(args[0]))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possibility that args could be empty and args[0] be out of bounds index access?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but this isn't represented in typing. values_of() will return None if there is no "arguments", so it needs at least 1 to have some value.

dfx/src/commands/canister/query.rs Outdated Show resolved Hide resolved
dfx/src/commands/canister/query.rs Outdated Show resolved Hide resolved
dfx/src/commands/canister/query.rs Outdated Show resolved Hide resolved
reject_code,
reject_message,
}) => Err(DfxError::ClientError(reject_code, reject_message)),
Ok(ReadResponse::Unknown) => Err(DfxError::Unknown("Unknown response".to_owned())),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is what we want to do here. The spec says:

Warning: Immediately after submitting a request, this may fail (e.g. return with unknown) even though the system is still working on accepting the request as pending.

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 was from the call method. I only moved the same logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it was wrong before as well

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 Query cannot return UNKNOWN, so I don't know why that's there. That would be a panic!("this should not happen"). I'll leave it here with a note that we should remove this response from query() when moving to the ic_http_api

Copy link
Contributor

Choose a reason for hiding this comment

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

In another PR I suggested having different response status types because some have pending and unknown and others don't. I think that's the way to go.

canister_id: CanisterId,
method_name: String,
arg: Option<Blob>,
) -> impl Future<Item = Option<Blob>, Error = DfxError> {
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 Item = Option<Blob> is incorrect since submit calls have no response body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this it can return an arg.

Copy link
Contributor

Choose a reason for hiding this comment

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

Requests to the submit endpoint can't return anything. What "Reply fields" means (as opposed to "Response fields" for a request to the read endpoint) is what fields will be present in the reply field when you make a subsequent request-status request and the status is replied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@hansl hansl force-pushed the hansl/canister-call branch 2 times, most recently from bc22970 to 30e4914 Compare September 20, 2019 23:12
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.

The return type of call still needs to be updated

@@ -1,3 +1,5 @@
pub mod api_client;
pub mod env;
pub mod error;

pub type CanisterId = u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we could use phantom_newtype from the DFINITY repo for things like this. I asked John a while ago and he said he'd think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

(No changes required here, just wanted to comment about it)

Copy link
Contributor Author

@hansl hansl Sep 24, 2019

Choose a reason for hiding this comment

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

Oh that's nice. Dunno why we shouldn't make that a public crate even, there's plenty of projects that could use newtype categories like that.

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 type is moved in my PR for the ic_http_api. I'll make it a new type there.

reject_code,
reject_message,
} => err(DfxError::ClientError(reject_code, reject_message)),
ReadResponse::Unknown => err(DfxError::Unknown("Unknown response".to_owned())),
Copy link
Contributor

@paulyoung paulyoung Sep 23, 2019

Choose a reason for hiding this comment

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

I still think we should change this to avoid problems when polling (maybe just println!("Unknown response")?) but up to you whether to do it 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.

I want to get rid of this command altogether. It probably shouldn't be part of the developer main experience, which should use the frontend to interact with the canister, or use lower level canister subcommands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the command in a commit.

@@ -108,6 +108,9 @@ fn random_blob() -> Blob {

/// A read request. Intended to remain private in favor of exposing specialized
/// functions like `query` instead.
///
/// TODO: filter the output of this function when moving to ic_http_api.
/// For example, it should never return Unknown or Pending, per the spec.
Copy link
Contributor

Choose a reason for hiding this comment

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

To reiterate what I said earlier: #36 (comment)

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)

hansl and others added 5 commits September 24, 2019 11:32
And move the is_number into it, with is_canister instead to allow for
canister ID proper typing.
DEPRECATION: please use "cannister call" instead
@hansl hansl merged commit b67f56e into master Sep 24, 2019
@hansl hansl deleted the hansl/canister-call branch October 10, 2019 21:48
hansl added a commit that referenced this pull request Nov 9, 2020
commit 7c5e71488df430b89de637a15b56814db196e752
Author: Islam El-Ashi <ielashi@users.noreply.github.com>
Date:   Sat Nov 7 00:30:05 2020 +0100

    feat: DER-encode public key and principal ID when using Plain Authentication. (#48)

    BREAKING CHANGE

    If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed.

commit a254861ae4bfe608734c8c190e4033cfe39f047b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:32 2020 -0800

    chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49)

    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

    Signed-off-by: dependabot[bot] <support@github.com>

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 694849109bc28fea80119388504127e889b36d15
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:14 2020 -0800

    chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50)

    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

    Signed-off-by: dependabot[bot] <support@github.com>

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:16:53 2020 -0800

    chore(deps-dev): bump node-fetch in /packages/bootstrap (#51)

    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

    Signed-off-by: dependabot[bot] <support@github.com>

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit ba4b082e5795150176f7adad989c3613edceb85f
Author: Hans <hans@larsen.online>
Date:   Thu Nov 5 12:49:59 2020 -0800

    test: add ic-ref, ci e2e and lerna support to repo (#47)

    This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection).

commit 8baa1cfc227d003c2806df1057b92009159df250
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:36 2020 -0800

    feat: use anonymous principal by default in HttpAgent (#46)

    If the auth transform is missing and the principal is anonymous, use an
    anonymous auth transform. Otherwise, use the auth transform as normal.

    # BREAKING CHANGE
    Before, an auth transform would not be called if it was missing, now it
    results as an error if the principal is not anonymous. This was because
    just using the packet as is without putting it in an envelope was a bug
    and should never be sent to a replica anyway. Now this is explicit.

commit 1a714e6c53f5e467112b964b71204b6d2eb545ef
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:15 2020 -0800

    feat: add a getPrincipal() method to Agents (#40)

commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 08:49:44 2020 -0800

    feat: add the canister ID to the window.ic object (#41)

    This can be useful to know the canister ID of the current frontend.

commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919
Author: Hans <hans@larsen.online>
Date:   Mon Nov 2 21:57:21 2020 -0800

    feat: add a cache bust hash at the end of javascript output (#39)

commit 9f2ed968b7cc9f4b854af5e23edca19d34124909
Author: Benjamin Goering <benjamin.goering@dfinity.org>
Date:   Tue Sep 22 15:28:49 2020 -0700

    docs: polish docs/npm-publish.md per @hansl feedback (#37)

    via: pr feedback on dfinity/agent-js#36
mergify bot pushed a commit that referenced this pull request Nov 10, 2020
commit 19c28008db5fa1582da573088b17d0bdee9e8bc2
Author: Hans <hans@larsen.online>
Date:   Mon Nov 9 14:47:25 2020 -0800

    revert: Remove lerna (PR #47) (#59)
    
    This removes lerna, but attempt to not remove the e2e tests and the
    new packages/files that were added after the last PR.

commit 426a94f356bc4e27f50c8bec55071b82105adbd1
Author: Hans <hans@larsen.online>
Date:   Mon Nov 9 13:54:04 2020 -0800

    feat: use anonymous for the first retrieve (#53)
    
    Also include some cleanup of the scripts that arent used, and add a diagram for the
    bootstrapping process.
    
    This does not affect the worker yet. Only the first retrieve of the index.js on
    localhost.
    
    BREAKING CHANGE
    
    If a canister has a retrieve() method that relies on Principal, the principal used
    will change.
    
    Co-authored-by: Benjamin Goering <benjamin.goering@dfinity.org>

commit b795d8b34ce38dd35e1bcf2b80e33108cf74161d
Author: Andrew Wylde <2126677+codelemur@users.noreply.github.com>
Date:   Mon Nov 9 09:41:13 2020 -0800

    feat(idp): add identity-provider package to repository (#52)
    
    adds plain HTML/ts and test setup for project

commit 7c5e71488df430b89de637a15b56814db196e752
Author: Islam El-Ashi <ielashi@users.noreply.github.com>
Date:   Sat Nov 7 00:30:05 2020 +0100

    feat: DER-encode public key and principal ID when using Plain Authentication. (#48)
    
    BREAKING CHANGE
    
    If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed.

commit a254861ae4bfe608734c8c190e4033cfe39f047b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:32 2020 -0800

    chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 694849109bc28fea80119388504127e889b36d15
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:14 2020 -0800

    chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:16:53 2020 -0800

    chore(deps-dev): bump node-fetch in /packages/bootstrap (#51)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit ba4b082e5795150176f7adad989c3613edceb85f
Author: Hans <hans@larsen.online>
Date:   Thu Nov 5 12:49:59 2020 -0800

    test: add ic-ref, ci e2e and lerna support to repo (#47)
    
    This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection).

commit 8baa1cfc227d003c2806df1057b92009159df250
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:36 2020 -0800

    feat: use anonymous principal by default in HttpAgent (#46)
    
    If the auth transform is missing and the principal is anonymous, use an
    anonymous auth transform. Otherwise, use the auth transform as normal.
    
    # BREAKING CHANGE
    Before, an auth transform would not be called if it was missing, now it
    results as an error if the principal is not anonymous. This was because
    just using the packet as is without putting it in an envelope was a bug
    and should never be sent to a replica anyway. Now this is explicit.

commit 1a714e6c53f5e467112b964b71204b6d2eb545ef
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:15 2020 -0800

    feat: add a getPrincipal() method to Agents (#40)

commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 08:49:44 2020 -0800

    feat: add the canister ID to the window.ic object (#41)
    
    This can be useful to know the canister ID of the current frontend.

commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919
Author: Hans <hans@larsen.online>
Date:   Mon Nov 2 21:57:21 2020 -0800

    feat: add a cache bust hash at the end of javascript output (#39)

commit 9f2ed968b7cc9f4b854af5e23edca19d34124909
Author: Benjamin Goering <benjamin.goering@dfinity.org>
Date:   Tue Sep 22 15:28:49 2020 -0700

    docs: polish docs/npm-publish.md per @hansl feedback (#37)
    
    via: pr feedback on dfinity/agent-js#36
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.

2 participants