Userlib: CanisterIds are little endian by nomeata · Pull Request #317 · dfinity/sdk

nomeata · 2020-01-15T09:10:32Z

this is the equivalent of what #303 did to dfx: When CBOR-encoding a
canisterid for submission in a request, we need to make sure it is
little endian.

We do the change in

class CanisterIdEncoder implements CborEncoder<CanisterId> { …

where it belongs: As close to the HTTP interface we can.

This means that once we migrate to
https://github.com/dfinity-lab/dfinity/pull/2286
we will adjust this code to switch to CBOR blob, and the work-around can
be dropped here. Note in the code about that.

Also cleans up fromText to be more robust: It will only accept ic:
textual representation, and always strip the checksum.

With this, I can

build and install Linkedup
copy the ic: url printed by dfx upon installation
use that in the url (e.g. http://127.0.0.1:8000/?canisterId=ic:72028DEDFB546FA89A)
see it load the assets

ggreif

Were your examples on purpose? Otherwise better fix, or add TODOs.

src/userlib/js/src/cbor.test.ts

nomeata · 2020-01-15T09:36:26Z

The userlib doesn't check checksums and ignores case, so I was just doing the minimal needed. But since you calculated the checksums anyways, I might as well fix this, thanks!

src/userlib/js/src/cbor.test.ts

eftychis · 2020-01-15T09:51:33Z

src/userlib/js/src/cbor.ts

+
+  // Drop once we use https://github.com/dfinity-lab/dfinity/pull/2286
+  private changeEndianness(str: string): string {
+    const result = [];


I did this instead:

const r = str.match(/../g); if (!r) { throw new Error( 'Failed to fix the canister id '+str ); } // Reverse const correct_cid=r.reverse().join(''); return correct_cid;

I also modified the test in cbor.test.ts to check for endianess, but it doesn't make sense I think, since we should soon remove this.

This might have issues when we receive the canister id, but I don't think this is used in the LinkedUp demo (as this is not inverse of toHex().

cc @stanleygjones

Correct; Canister ids are only recevied upon canister registration, this is not something the user lib does, so we are good on that front.

Agree that your code is a bit more elegant, I just copied the nible-reversing code from @chenyan-dfinity. But it’ll be removed in a few days anyways, so let’s not worry.

src/userlib/js/src/http_agent.test.ts

eftychis

cc @stanleygjones

src/userlib/js/src/http_agent.test.ts

src/userlib/js/src/actor.test.ts

eftychis · 2020-01-15T10:05:30Z

src/userlib/js/types/borc.d.ts

-      size: Number,
-      tags: Record<number, (val: any) => any>,
-    });
+    constructor(opts: { size: Number; tags: Record<number, (val: any) => any> });


does the style checker pass?

This is the result of running the style checker here…

IC checks the style checker, so I guess the answer is yes?

this is the equivalent of what #303 did to `dfx`: When CBOR-encoding a canisterid for submission in a request, we need to make sure it is little endian. We do the change in ``` class CanisterIdEncoder implements CborEncoder<CanisterId> { … ``` where it belongs: As close to the HTTP interface we can. This means that once we migrate to https://github.com/dfinity-lab/dfinity/pull/2286 we will adjust this code to switch to CBOR blob, and the work-around can be dropped here. Note in the code about that. Also cleans up `fromText` to be more robust: It will _only_ accept `ic:` textual representation, and _always_ strip the checksum. With this, I can * build and install Linkedup * copy the `ic:` url printed by `dfx` upon installation * use that in the url (e.g. `http://127.0.0.1:8000/?canisterId=ic:72028DEDFB546FA89A`) * see it load the assets

nomeata · 2020-01-15T11:12:23Z

I am reluctant to merge this into another team’s repo, and will leave it to someone on the SDK team to press the button.

Oh well, it has worked on Stanley’s and my machine, clearly that’s progress. And no place like master.

nomeata · 2020-01-15T16:00:51Z

Follow up in #319

chenyan-dfinity · 2020-01-15T16:47:45Z

src/userlib/js/src/cbor.ts


  public encode(v: CanisterId): cbor.CborValue {
-    return cbor.value.u64(v.toHex(), 16);
+    return cbor.value.u64(this.changeEndianness(v.toHex()), 16);


Can we changeEndianness inside toHex function?

No! The toHex is the correct CanisterID to blob conversion that we will want to use once we update dfinity. We should keep the work-around close the CBOR encoding, where we shoehorn a blob into a u64.

This reverts commit a294d27.

nomeata requested review from a user, chenyan-dfinity and eftychis January 15, 2020 09:10

nomeata requested a review from a team as a code owner January 15, 2020 09:10

nomeata mentioned this pull request Jan 15, 2020

fix endian in userlib #314

Closed

ggreif changed the title ~~Userlib: CaniterIds are little endian~~ Userlib: CanisterIds are little endian Jan 15, 2020

ggreif reviewed Jan 15, 2020

View reviewed changes

src/userlib/js/src/cbor.test.ts Outdated Show resolved Hide resolved

src/userlib/js/src/cbor.test.ts Outdated Show resolved Hide resolved

nomeata commented Jan 15, 2020

View reviewed changes

src/userlib/js/src/cbor.test.ts Outdated Show resolved Hide resolved

eftychis reviewed Jan 15, 2020

View reviewed changes

nomeata commented Jan 15, 2020

View reviewed changes

src/userlib/js/src/http_agent.test.ts Outdated Show resolved Hide resolved

eftychis approved these changes Jan 15, 2020

View reviewed changes

nomeata commented Jan 15, 2020

View reviewed changes

src/userlib/js/src/http_agent.test.ts Outdated Show resolved Hide resolved

nomeata commented Jan 15, 2020

View reviewed changes

src/userlib/js/src/actor.test.ts Outdated Show resolved Hide resolved

eftychis reviewed Jan 15, 2020

View reviewed changes

nomeata force-pushed the joachim/userlib-little-endian branch from 8333432 to a1fee1f Compare January 15, 2020 10:08

nomeata force-pushed the joachim/userlib-little-endian branch 3 times, most recently from c1497a9 to 0dc4abe Compare January 15, 2020 10:14

nomeata merged commit a294d27 into master Jan 15, 2020

nomeata deleted the joachim/userlib-little-endian branch January 15, 2020 15:49

chenyan-dfinity reviewed Jan 15, 2020

View reviewed changes

chenyan-dfinity mentioned this pull request Jan 15, 2020

Userlib: Fix endianness in request id calculation #319

Merged

eftychis added a commit that referenced this pull request Jan 29, 2020

Revert "Userlib: CaniterIds are little endian (#317)"

105e058

This reverts commit a294d27.

Comments

Conversation

nomeata commented Jan 15, 2020

Uh oh!

ggreif left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nomeata commented Jan 15, 2020

Uh oh!

Uh oh!

eftychis Jan 15, 2020 • 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!

eftychis Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

nomeata Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eftychis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eftychis Jan 15, 2020 • 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!

nomeata Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

nomeata commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nomeata commented Jan 15, 2020

Uh oh!

chenyan-dfinity Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

nomeata Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eftychis Jan 15, 2020 •

edited

Loading

eftychis Jan 15, 2020 •

edited

Loading

nomeata commented Jan 15, 2020 •

edited

Loading