Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

web3.js: use Uint8Array instead of Buffer for Transaction.addSignature#27861

Closed
macalinao wants to merge 1 commit intosolana-labs:masterfrom
saber-hq:igm/no-buffer-for-addsignature
Closed

web3.js: use Uint8Array instead of Buffer for Transaction.addSignature#27861
macalinao wants to merge 1 commit intosolana-labs:masterfrom
saber-hq:igm/no-buffer-for-addsignature

Conversation

@macalinao
Copy link
Copy Markdown
Contributor

Problem

Buffer is used where Uint8Array is sufficient, making Browserify required for simple libraries.

See: https://twitter.com/armaniferrante/status/1569871757687029761

@mergify mergify Bot added the community Community contribution label Sep 17, 2022
@mergify mergify Bot requested a review from a team September 17, 2022 22:57
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 23, 2022

Codecov Report

Merging #27861 (b7759d4) into master (e6687b8) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #27861   +/-   ##
=======================================
  Coverage    77.3%    77.4%           
=======================================
  Files          55       55           
  Lines        2889     2910   +21     
  Branches      402      404    +2     
=======================================
+ Hits         2234     2253   +19     
- Misses        514      515    +1     
- Partials      141      142    +1     

@jordaaash
Copy link
Copy Markdown
Contributor

This looks fine to me, but this will fail commitlint, along with all the other PRs. I have a few asks --

  1. Can you prefix the commit messages so CI will run? fix: should be fine for nonbreaking stuff.
  2. Can you mark PRs that are breaking? For example, a change to arguments from (bytes: Buffer) to (bytes: Uint8Array) is nonbreaking, but changing the types/returns of public properties/methods is, so we have to figure out what to do about that.
  3. Can you logically group these PRs somewhat? Reviewing like 20 PRs, many of which are related, is tougher to do in isolation.

@macalinao
Copy link
Copy Markdown
Contributor Author

This looks fine to me, but this will fail commitlint, along with all the other PRs. I have a few asks --

  1. Can you prefix the commit messages so CI will run? fix: should be fine for nonbreaking stuff.

Sure

  1. Can you mark PRs that are breaking? For example, a change to arguments from (bytes: Buffer) to (bytes: Uint8Array) is nonbreaking, but changing the types/returns of public properties/methods is, so we have to figure out what to do about that.

Sure, will use conventional commits here

  1. Can you logically group these PRs somewhat? Reviewing like 20 PRs, many of which are related, is tougher to do in isolation.

What grouping would give the PRs the highest probability of being accepted?

@jordaaash
Copy link
Copy Markdown
Contributor

What grouping would give the PRs the highest probability of being accepted?

Well... it's hard to say without looking at all of them! I'd say maybe start by grouping the ones having to do with Buffer -> Uint8Array together, especially any that are breaking. There are some non-obvious ways a change like this can be breaking: #27700

In principle, I think we're all for getting away from Buffer, but I think we have to see if ultimately it's going to mean shipping web3.js v2.0 just for this.

Copy link
Copy Markdown
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Begone, typed array!

const signature = byteArray.slice(0, SIGNATURE_LENGTH_IN_BYTES);
byteArray = byteArray.slice(SIGNATURE_LENGTH_IN_BYTES);
signatures.push(bs58.encode(Buffer.from(signature)));
signatures.push(bs58.encode(Uint8Array.from(signature)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can go straight into bs58.encode() as an array of numbers!

Suggested change
signatures.push(bs58.encode(Uint8Array.from(signature)));
signatures.push(bs58.encode(signature));

@github-actions github-actions Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 29, 2022
@github-actions github-actions Bot closed this Jan 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants