Skip to content

Userlib: CanisterIds are little endian #317

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.

Sign up for GitHub

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

Merged
nomeata merged 1 commit into from
Jan 15, 2020
Merged

Userlib: CanisterIds are little endian #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/userlib/js/src/actor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ test('makeActor', async () => {

const arg = blobFromHex(IDL.encode([IDL.Text], [argValue]).toString('hex'));

const canisterId: CanisterId = CanisterId.fromText('0000000000000001');
const canisterId: CanisterId = CanisterId.fromText('ic:000000000000000107');
const senderPubKey = Buffer.alloc(32, 0) as SenderPubKey;
const senderSecretKey = Buffer.alloc(32, 0) as SenderSecretKey;
const senderSig = Buffer.from([0]) as SenderSig;
Expand Down
6 changes: 3 additions & 3 deletions src/userlib/js/src/canisterId.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ export class CanisterId {
if (hex.startsWith('ic:')) {
// Remove the checksum from the hexadecimal.
// TODO: validate the checksum.
return CanisterId.fromText(hex.slice(3, -2));
return new this(hex.slice(3, -2));
} else {
throw new Error('CanisterId not a ic: url: ' + hex);
}

return new this(hex.padStart(16, '0'));
}

protected constructor(private _idHex: string) {}
Expand Down
4 changes: 2 additions & 2 deletions src/userlib/js/src/cbor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ test('round trip', () => {
b: 'two',
c: Buffer.from([3]) as BinaryBlob,
d: { four: 'four' },
e: CanisterId.fromText('ffffffffffffffff'),
e: CanisterId.fromText('ic:FFFFFFFFFFFFFFFFD7'),
f: Buffer.from([]) as BinaryBlob,
g: new BigNumber('0xffffffffffffffff'),
};
Expand All @@ -37,6 +37,6 @@ test('round trip', () => {
const { c: outputC, e: outputE, f: outputF, ...outputRest } = output;

expect(blobToHex(outputC)).toBe(blobToHex(inputC));
expect(((outputE as any) as BigNumber).toString(16)).toBe(inputE.toHex());
expect(((outputE as any) as BigNumber).toString(16).toUpperCase()).toBe(inputE.toHex());
expect(outputRest).toEqual(inputRest);
});
13 changes: 12 additions & 1 deletion src/userlib/js/src/cbor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,18 @@ class CanisterIdEncoder implements CborEncoder<CanisterId> {
}

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

@chenyan-dfinity chenyan-dfinity Jan 15, 2020

Choose a reason for hiding this comment

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

Can we changeEndianness inside toHex function?

Copy link
Contributor Author

@nomeata nomeata Jan 16, 2020

Choose a reason for hiding this comment

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

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.

}

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

@eftychis eftychis Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

@eftychis eftychis Jan 15, 2020

Choose a reason for hiding this comment

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

cc @stanleygjones

Copy link
Contributor Author

@nomeata nomeata Jan 15, 2020

Choose a reason for hiding this comment

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

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.

let len = str.length - 2;
while (len >= 0) {
result.push(str.substr(len, 2));
len -= 2;
}
return result.join('');
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/userlib/js/src/http_agent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ test('call', async () => {
);
});

const canisterId: CanisterId = CanisterId.fromText('0000000000000001');
const canisterId: CanisterId = CanisterId.fromText('ic:000000000000000107');
const nonce = Buffer.from([0, 1, 2, 3, 4, 5, 6, 7]) as Nonce;
// prettier-ignore
const seed = Buffer.from([
Expand Down Expand Up @@ -102,7 +102,7 @@ test('requestStatus', async () => {
);
});

const canisterIdent = '0000000000000001';
const canisterIdent = 'ic:000000000000000107';
const nonce = Buffer.from([0, 1, 2, 3, 4, 5, 6, 7]) as Nonce;

// prettier-ignore
Expand Down
9 changes: 3 additions & 6 deletions src/userlib/js/types/borc.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
declare module "borc" {
import { Buffer } from "buffer/";
declare module 'borc' {
import { Buffer } from 'buffer/';

class Decoder {
constructor(opts: {
size: Number,
tags: Record<number, (val: any) => any>,
});
constructor(opts: { size: Number; tags: Record<number, (val: any) => any> });
Copy link
Contributor

@eftychis eftychis Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

does the style checker pass?

Copy link
Contributor Author

@nomeata nomeata Jan 15, 2020

Choose a reason for hiding this comment

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

This is the result of running the style checker here…

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


decodeFirst(input: ArrayBuffer): any;
}
Expand Down
8 changes: 4 additions & 4 deletions src/userlib/js/types/buffer-pipe.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
declare module 'buffer-pipe' {
import { Buffer } from "buffer/";
import { Buffer } from 'buffer/';

class BufferPipe {
readonly buffer: Buffer;
Expand Down Expand Up @@ -27,19 +27,19 @@ declare module 'buffer-pipe' {
* Whether or not there is more data to read from the buffer
* returns {Boolean}
*/
get end (): boolean;
get end(): boolean;

/**
* returns the number of bytes read from the stream
* @return {Integer}
*/
get bytesRead (): number;
get bytesRead(): number;

/**
* returns the number of bytes wrote to the stream
* @return {Integer}
*/
get bytesWrote (): number;
get bytesWrote(): number;
}

export = BufferPipe;
Expand Down