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.

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

migrate from buffer to Uint8Array #52

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

barrytra
Copy link

@barrytra barrytra commented Apr 9, 2024

the following PR fixes #51 .
test script contained some instance of buffer which i shifted to Uint8Array.
uint8array-tools library has also been used to make things easier.

@barrytra
Copy link
Author

barrytra commented Apr 9, 2024

I have kept naming conventions as it is.. LMK if that has to be changed @junderw

Copy link
Member

@junderw junderw left a comment

Choose a reason for hiding this comment

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

Thanks for getting started.

Here are a few suggestions to get the ball rolling.

You can also start a pull request to add any other tools you think might be useful (that you see used a lot with Buffers, but can't find an equivalent for Uint8Array) to the uint8array-tools library.

const bytes = Buffer.from(bech32.fromWords(f.words));
const bytes2 = Buffer.from(bech32.fromWordsUnsafe(f.words));
const words = bech32.toWords(uint8arraytools.fromHex(f.hex));
const bytes = new Uint8Array(Buffer.from(bech32.fromWords(f.words)));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const bytes = new Uint8Array(Buffer.from(bech32.fromWords(f.words)));
const bytes = Uint8Array.from(bech32.fromWords(f.words));

Copy link
Author

Choose a reason for hiding this comment

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

the change that u suggested would give a 0 value for any latin character. we will have to initialise a new Uint8Array instance.
or write it like:

 const bytes = Uint8Array.from(Buffer.from(bech32.fromWords(f.words)));

If we want to completely eliminate every instance of buffer here, one thing I can do is create a function for the same in uint8array-tools library or keep it the same as i did.

t.same(words, f.words);
t.same(bytes.toString('hex'), f.hex);
t.same(bytes2.toString('hex'), f.hex);
t.same(Buffer.from(bytes).toString('hex'), f.hex);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
t.same(Buffer.from(bytes).toString('hex'), f.hex);
t.same(uint8arraytools.toHex(bytes), f.hex);

@barrytra
Copy link
Author

barrytra commented Apr 9, 2024

also in code there are requirements to convert typedArray to string in 'utf8' or 'binary' format. Whereas uint8array-tools library only provides to convert it to 'hex' format. I'll proceed with adding those two feature as well in that library

@barrytra
Copy link
Author

barrytra commented Apr 13, 2024

Hey @junderw all requested changes have been made.
For now i Have installed "uint8array-tools" library from my forked repo.
please review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending Review
Development

Successfully merging this pull request may close these issues.

Upgrading from buffer to Uint8Array.
2 participants