Skip to content
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

Jump to bottom

getNumber() doesn't work correctly #50

Closed
vmx opened this issue Sep 19, 2019 · 2 comments · Fixed by #65
Closed

getNumber() doesn't work correctly #50

vmx opened this issue Sep 19, 2019 · 2 comments · Fixed by #65

Comments

@vmx
Copy link
Member

vmx commented Sep 19, 2019

The getNumber method is broken and doesn't work for codecs > 1 byte.

Example:

> multicodec.getNumber('blake2b-8')
178
> multicodec.BLAKE2B_8
45569
@JonasKruckenberg
Copy link
Contributor

Can we get this fixed somehow ? It completely breaks the ipld formats in ipfs-http-client
It seems just ommitting the varint encoding asnd decoding does the trick...
Why is it even varint encoded in the first place? the .getCodeVarint function can just encode the normal js number to an varint for that feature to work

@JonasKruckenberg JonasKruckenberg mentioned this issue Sep 3, 2020
Merged
@JonasKruckenberg
Copy link
Contributor

Nevermind, I now know what the encoding is used for, I think i fixed it regardless.
I also added a test that tests against all codec names not just a few

hacdias pushed a commit that referenced this issue Sep 7, 2020
@JonasKruckenberg
fix: getNumber varint decode bug
ce93ceb 
This fixes #50 by removing the varintUint8ArrayDecode function and using varint.decode directly instead.

Co-authored-by: Jonas Kruckenberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: getNumber varint decode bug
2 participants
@vmx @JonasKruckenberg