First Public Spec library draft and Request ID calculation in Rust #42

hansl · 2019-09-23T04:35:42Z

This is diff based onto PR #41, so commit 8f9b127 does not need to be reviewed.

feat: add public_spec library and Request Id serializer …
The public_spec library would be ideal to separate the CLI with parts that
can be reused if interacting with the public spec. Its also a good way to
formalize the public spec in Rust for us.

The serializer is a simple Serde serializer that errors for everything
except messages of supported types and forms (no recursive structures, etc).

refactor: use the public_spec exported types in api_client

TODO after this:

Move more things to the public spec (SDK-446) (not an AI for this PR)
Use the Request ID for request-status messages.
Manual testing that calls are made and requested status matches, once we can actually install canisters WASM that doesn't break.
Add nix support for lib/public_spec for at least running CI on it.

hansl · 2019-09-23T04:41:11Z

@paulyoung Let's sync tomorrow for solving the Nix issues. I think those should be fixed in my other PR then rebased here.

nomeata · 2019-09-23T07:01:53Z

The public_spec library would be ideal to separate the CLI with parts that
can be reused if interacting with the public spec. Its also a good way to
formalize the public spec in Rust for us.

Nice idea. But I think “public_spec” is a bad name (are people starting to use “public spec” as a name, rather than a description). Maybe “ic-http-api”?

hansl · 2019-09-23T14:53:45Z

@nomeata What do you think about a more direct ic-http-client?

paulyoung · 2019-09-23T18:41:31Z

Nice idea. But I think “public_spec” is a bad name (are people starting to use “public spec” as a name, rather than a description). Maybe “ic-http-api”?

I think renaming the document would help me to know how to refer to it

paulyoung · 2019-09-23T18:41:57Z

@hansl said this is still WIP so I'm going to hold off on reviewing this for now

nomeata · 2019-09-24T07:23:46Z

ic-http-client is great

paulyoung

I still recognize that this is WIP. Commenting so that the Pull Reminders bot will leave me alone :)

hansl · 2019-09-30T21:13:35Z

I'm getting real close to a reviewable PR.

It was committed by mistake.

hansl · 2019-10-01T01:42:17Z

@paulyoung Please review this PR. This is ready.

hansl · 2019-10-01T15:23:36Z

@paulyoung This PR can be reviewed without the OSX build passing. I'm investigating it but the Linux one is, so the code is valid, it might just be Nix acting differently.

matthewhammer

I am trying to help unblock @hansl

So, I am giving an approval here without high confidence or a deep review.

dfx/Cargo.toml

lib/ic_http_agent/src/types/blob.rs

paulyoung · 2019-10-01T18:14:25Z

lib/ic_http_agent/src/types/canister_id.rs

+/// code samples (including this library). For now, we newtype it to abstract its usage
+/// from a number, and will change its internal type when time comes.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct CanisterId(Blob);


"Using Rust at DFINITY" mentions this crate: https://docs.rs/newtype_derive/0.1.6/newtype_derive/

It seems useful in general but not sure if it's worth adding in this case.

It doesn't seem used anymore. Can't find any mention of custom_derive in the rust folder.

lib/ic_http_agent/src/types/canister_id.rs

e2e/basic-project.bash

paulyoung · 2019-10-01T21:34:33Z

nix/overlays/dfinity-sdk.nix

          name = "${oldAttrs.name}-debug";
        });
        rust-workspace-doc = rust-workspace-debug.doc;
+        e2e-tests = super.callPackage ../e2e-tests.nix {};


These e2e tests are specific to dfx so I think they should be named that way.

The goal here is to have global e2e tests, including frontend ones. So I'd say it's not scoped to dfx. Right now they are.

My point is that if I want to add end-to-end tests for the JS user library, I don't think they belong in this file

I disagree in the sense that dfinity-sdk.packages.e2e-tests should cover all e2e tests.

I'm also not 100% sure what you mean here. Are you saying that nix/overlays/dfinity-sdk.nix should not have JavaScript things in it? I was working with the impression all packages should be defined here, and those used by whatever layers should be referenced from their directory, here. Essentially you would have a default.nix in the userlib/js/ folder (or wherever) that points to dfinity-sdk.packages.userlib.js. That's what I thought was the plan.

I'm not saying that the SDK overlay shouldn't have JavaScript things in it. I'm saying that I anticipate end-to-end tests for dfx to be quite different to end-to-end tests for other parts of the SDK.

I think this is influenced by older ideas about what would be in the SDK (e.g. a "Try ActorScript in a browser" type tool I was working on) so happy to leave this as-is for now.

I see. Yes, that makes a lot of sense. When it happens I'd split e2e-tests into multiple subpackages, and have e2e-tests be a set of those.

nix/rust-workspace.nix

hansl · 2019-10-01T23:01:47Z

@paulyoung Thanks a lot for the valuable feedback. Please take another look, I think I answered all your comments (sorry if I missed one).

dfx/src/commands/canister/request_status.rs

hansl · 2019-10-02T00:18:11Z

Does this mean we can drop the dev dependency?

Which dev dependency?

paulyoung · 2019-10-02T00:22:37Z

Does this mean we can drop the dev dependency?

Which dev dependency?

I thought at one point rand was only added as a dev dependency but it must be used elsewhere too.

hansl · 2019-10-02T00:26:23Z

rand

It's used when generating a nonce.

hansl · 2019-10-02T02:34:59Z

@paulyoung Am I missing anything left on this PR?

It creates the types for RequestId (and serializes it), CanisterId and Blob. Allows for serde serialization of all.

Also move every usage of RequestId, CanisterId and Blob to ic-http-agent.

It handles request ID and other improvements.

With a few starter tests.

We should keep arguments (at least deps) sorted to facilitate merges. Also we should not depend on the -debug build of DFINITY. We should use -debug for debug build, otherwise release build.

hansl · 2019-10-02T17:46:08Z

Just rebased everything, forced push, waiting for CI then merging.

hansl requested a review from paulyoung September 23, 2019 04:35

hansl mentioned this pull request Sep 23, 2019

IDL Deserialization for Rust #40

Merged

5 tasks

hansl force-pushed the hansl/message-id branch from 4767cec to 445b539 Compare September 24, 2019 17:15

paulyoung reviewed Sep 24, 2019

View reviewed changes

hansl force-pushed the hansl/message-id branch 3 times, most recently from 7b0de34 to 30e5960 Compare September 27, 2019 16:25

hansl requested a review from a team as a code owner September 29, 2019 05:30

hansl force-pushed the hansl/message-id branch from b00319c to 3712121 Compare September 30, 2019 20:19

hansl force-pushed the hansl/message-id branch from 3712121 to 0482f4e Compare September 30, 2019 23:07

build: remove unused Cargo.toml section

cfa54a1

It was committed by mistake.

hansl force-pushed the hansl/message-id branch 2 times, most recently from 3c88565 to a10ea84 Compare October 1, 2019 01:41

hansl requested a review from paulyoung October 1, 2019 01:42

matthewhammer approved these changes Oct 1, 2019

View reviewed changes

paulyoung suggested changes Oct 1, 2019

View reviewed changes

hansl force-pushed the hansl/message-id branch from 4f1444f to abf94f4 Compare October 1, 2019 22:58

hansl requested a review from paulyoung October 1, 2019 23:01

paulyoung reviewed Oct 1, 2019

View reviewed changes

dfx/src/commands/canister/request_status.rs Outdated Show resolved Hide resolved

paulyoung reviewed Oct 1, 2019

View reviewed changes

dfx/src/commands/canister/request_status.rs Show resolved Hide resolved

hansl requested a review from paulyoung October 2, 2019 16:37

paulyoung approved these changes Oct 2, 2019

View reviewed changes

hansl added 6 commits October 2, 2019 10:44

feat: add ic-http-agent library

ce5921d

It creates the types for RequestId (and serializes it), CanisterId and Blob. Allows for serde serialization of all.

feat: add canister request id in call

a93dc67

Also move every usage of RequestId, CanisterId and Blob to ic-http-agent.

deps: update latest dfinity

3ea467d

It handles request ID and other improvements.

tests: add e2e tests using bats

e682058

With a few starter tests.

build: cleanup rust-workspace.nix

5bf7261

We should keep arguments (at least deps) sorted to facilitate merges. Also we should not depend on the -debug build of DFINITY. We should use -debug for debug build, otherwise release build.

ci: use the dfinity build from the current system

f39b5e2

hansl force-pushed the hansl/message-id branch from 9c0b8e6 to f39b5e2 Compare October 2, 2019 17:46

hansl merged commit dd01186 into master Oct 2, 2019

hansl deleted the hansl/message-id branch October 2, 2019 18:02

First Public Spec library draft and Request ID calculation in Rust #42

First Public Spec library draft and Request ID calculation in Rust #42

Uh oh!

Conversation

hansl commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansl commented Sep 23, 2019

Uh oh!

nomeata commented Sep 23, 2019

Uh oh!

hansl commented Sep 23, 2019

Uh oh!

paulyoung commented Sep 23, 2019

Uh oh!

paulyoung commented Sep 23, 2019

Uh oh!

nomeata commented Sep 24, 2019

Uh oh!

paulyoung left a comment

Choose a reason for hiding this comment

Uh oh!

hansl commented Sep 30, 2019

Uh oh!

hansl commented Oct 1, 2019

Uh oh!

hansl commented Oct 1, 2019

Uh oh!

matthewhammer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hansl commented Oct 1, 2019

Uh oh!

Uh oh!

Uh oh!

hansl commented Oct 2, 2019

Uh oh!

paulyoung commented Oct 2, 2019

Uh oh!

hansl commented Oct 2, 2019

Uh oh!

hansl commented Oct 2, 2019

Uh oh!

hansl commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hansl commented Sep 23, 2019 •

edited

Loading