Skip to content

Comments

Endian 2nd try#320

Closed
chenyan-dfinity wants to merge 5 commits intomasterfrom
endian-2nd-try
Closed

Endian 2nd try#320
chenyan-dfinity wants to merge 5 commits intomasterfrom
endian-2nd-try

Conversation

@chenyan-dfinity
Copy link
Contributor

No description provided.

nomeata and others added 5 commits January 15, 2020 11:13
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
Copy link
Contributor

nomeata commented Jan 16, 2020

What is left do here? I.e. what doesn’t workon master (after #319 got merged)?

@chenyan-dfinity
Copy link
Contributor Author

Maybe not. This was created when I failed to properly build your fix. My matrix multiply example still doesn't work (canister id not found in client), but given that master works on both default app and linkedup, maybe it's just my javascript is bad.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

I don't think this is needed anymore, as we don't use endianness anywhere.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2020

Please re-open if you feel this should be updated and done.

@hansl hansl closed this Jan 24, 2020
@lwshang lwshang deleted the endian-2nd-try branch July 29, 2022 18:58
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.

3 participants