Add ability to produce a request ID from a request #74

paulyoung · 2019-10-11T02:14:56Z

Extracted from #56, with some changes.

This adds some minimal type information for requests for now.

js-user-library/src/canisterId.ts

paulyoung · 2019-10-11T18:18:11Z

Hydra is now somehow failing with an evaluation error:

hydra-eval-jobs returned exit code 1:
error: illegal name: '.drv'

paulyoung · 2019-10-11T18:21:14Z

But only on darwin...

js-user-library/src/requestId.test.ts

paulyoung · 2019-10-11T18:31:14Z

I'm now able to reproduce this locally:

error: while evaluating the attribute 'buildPhase' of the derivation 'dfinity-sdk-js-user-library' at /Users/paulyoung/projects/dfinity-lab/sdk/paulyoung/js-user-library-prs/js-user-library/package.nix:11:3:
while evaluating the attribute 'text' of the derivation 'npm-snapshot' at /nix/store/ga5ijbb5q9b5hknm2q5f3jvhsmfz0gn9-nixpkgs-patched/pkgs/build-support/trivial-builders.nix:7:14:
illegal name: '.drv'

We're using napalm (by @nmattia), and it looks like it's having trouble naming or producing a derivation from the package-lock.json file: https://github.com/nmattia/napalm/blob/e4da955b155e8d1bd1858a8479abe9ba9d846fe8/default.nix#L109-L110

I have no idea what triggered this.

eftychis · 2019-10-11T18:42:38Z

Is this possibly related to any upstream changes @paulyoung ?

paulyoung · 2019-10-11T18:51:33Z

Shouldn't be – everything is pinned 😕

paulyoung · 2019-10-11T18:56:34Z

Perhaps master is using a newer version of nixpkgs (via ~~dfinity/common~~ dfinity-lab/common) and that is somehow problematic for napalm

js-user-library/src/requestId.test.ts

paulyoung · 2019-10-11T19:14:54Z

I seem to have fixed the issue by regenerating the lock file 😑

eftychis · 2019-10-11T20:47:15Z

js-user-library/src/blob.ts

-  return new Uint8Array(Buffer.from(hex, "hex").buffer);
+// Named `BinaryBlob` as opposed to `Blob` so not to conflict with
+// https://developer.mozilla.org/en-US/docs/Web/API/Blob
+export type BinaryBlob = Uint8Array & { __blob__: void };


Interesting -- thanks Paul!

This is where I first read about this approach: microsoft/TypeScript#25709 (comment)

eftychis · 2019-10-11T20:54:47Z

js-user-library/package-lock.json

@@ -19,7 +19,7 @@
        "mkdirp": "^0.5.1",
        "output-file-sync": "^2.0.0",
        "slash": "^2.0.0",
-        "source-map": "^0.6.1"
+        "source-map": "^0.5.0"


Downgrading?

Not sure why but this is a generated file anyway.

js-user-library/src/request.ts

eftychis · 2019-10-11T20:59:01Z

js-user-library/src/request.ts

+  // NOTE: `nonce` is optional in the spec, but we provide it so that requests
+  // are unique and we avoid a bug in the client when the same request is
+  // submitted more than once: https://dfinity.atlassian.net/browse/DFN-895
+  nonce?: BinaryBlob;


eftychis · 2019-10-11T21:00:58Z

js-user-library/src/requestId.test.ts

+    sender_sig: new Uint8Array(64) as BinaryBlob,
+  };
+
+  const requestId = await requestIdOf(request);


Note to self (and anyone interested): I was hoping we have a way to call the client inside the tests and extend this test to incorporate the handler. However, after talking with @paulyoung it seems there is no easy and not time-consuming way to do this: deferred. 😢

To clarify, I was specifically talking about browser-based end-to-end tests. A Node.js version should be easier to get to sooner.

eftychis · 2019-10-11T21:02:19Z

js-user-library/src/requestId.ts

+};
+
+export const requestIdOf = async (request: Request): Promise<RequestId> => {
+  const { sender_pubkey, sender_sig, ...rest } = request;


nit: I would rename the rest -- usually with rest you get the impression that it is left out dropped. It is the other way around here though.

The remaining properties are called rest properties and I couldn't think of a better name at the time 😄

I'll rename it to fields.

eftychis · 2019-10-11T21:02:46Z

js-user-library/src/requestId.ts

+
+  const traversed: Array<[BinaryBlob, BinaryBlob]> = await Promise.all(hashed);
+
+  const sorted: Array<[BinaryBlob, BinaryBlob]> = traversed


Do we need the sorting here? (I am missing the reason here.)

determinism.

Edit: Apparently it is in the spec -- tt @paulyoung

eftychis

LGTM -- I have 1 question, 1 comment, and a really nit one (feel free to ignore it).

paulyoung added 2 commits October 10, 2019 19:13

Use a newtype for blobs

755dd3a

Add ability to produce a request ID from a request

e3bb174

paulyoung requested a review from a team as a code owner October 11, 2019 02:14

eftychis reviewed Oct 11, 2019

View reviewed changes

js-user-library/src/canisterId.ts Outdated Show resolved Hide resolved

paulyoung added 2 commits October 11, 2019 10:49

Add default.nix for building within the directory

9098bf0

Try regenerating package-lock.json to fix CI

749d0f4

paulyoung commented Oct 11, 2019

View reviewed changes

js-user-library/src/requestId.test.ts Outdated Show resolved Hide resolved

ghost reviewed Oct 11, 2019

View reviewed changes

js-user-library/src/requestId.test.ts Show resolved Hide resolved

paulyoung added 3 commits October 11, 2019 12:01

Remove commented out canister ID implementation

878afdd

Remove commented out import

ce75067

Clarify that request ID tests are from the spec

8d15809

paulyoung added 2 commits October 11, 2019 12:15

Regenerate package-lock.json again to fix napalm

460c77d

Merge branch 'master' into paulyoung/js-user-library-request-id

998c383

eftychis reviewed Oct 11, 2019

View reviewed changes

js-user-library/src/request.ts Outdated Show resolved Hide resolved

eftychis reviewed Oct 11, 2019

View reviewed changes

eftychis approved these changes Oct 11, 2019

View reviewed changes

paulyoung added 2 commits October 11, 2019 14:07

Reword comment about nonce since it matches spec

acb4339

Add TODO instead of commenting out request fields

5aa229a

Rename "rest" to "fields" for clarity

4ad5cc0

paulyoung added the automerge-squash label Oct 11, 2019

paulyoung merged commit 815942a into master Oct 11, 2019

paulyoung deleted the paulyoung/js-user-library-request-id branch October 11, 2019 21:31

mergify bot removed the automerge-squash label Oct 11, 2019


		const traversed: Array<[BinaryBlob, BinaryBlob]> = await Promise.all(hashed);

		const sorted: Array<[BinaryBlob, BinaryBlob]> = traversed

Add ability to produce a request ID from a request #74

Add ability to produce a request ID from a request #74

Uh oh!

Conversation

paulyoung commented Oct 11, 2019

Uh oh!

Uh oh!

paulyoung commented Oct 11, 2019

Uh oh!

paulyoung commented Oct 11, 2019

Uh oh!

Uh oh!

paulyoung commented Oct 11, 2019

Uh oh!

eftychis commented Oct 11, 2019

Uh oh!

paulyoung commented Oct 11, 2019

Uh oh!

paulyoung commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

paulyoung commented Oct 11, 2019

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!

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!

eftychis Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eftychis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulyoung commented Oct 11, 2019 •

edited

Loading

eftychis Oct 11, 2019 •

edited

Loading