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

fix: make toBytes actually return the type it's typehint claims #29313

Merged
merged 1 commit into from Dec 20, 2022

Conversation

cryptopapi997
Copy link
Contributor

Problem

toBytes claims to return a Uint8Array when in reality it returns a Buffer. This isn't a huge problem as in 99.9% of cases these two work just as well for the caller, but causes issues in some edge cases (e.g. toString() on a Uint8Array and on a Buffer over the same bytes returns different strings). It's also a code smell, due to making this method fully redundant to the toBuffer() method in addition to its typehint not returning exactly what it says it should.

Summary of Changes

Actually convert the buffer to a Uint8Array

@github-actions github-actions bot added the web3.js Related to the JavaScript client label Dec 17, 2022
@mergify mergify bot added the community Community contribution label Dec 17, 2022
@mergify mergify bot requested a review from a team December 17, 2022 13:53
@codecov
Copy link

codecov bot commented Dec 17, 2022

Codecov Report

Merging #29313 (a773f3b) into master (4267a15) will decrease coverage by 0.5%.
The diff coverage is n/a.

❗ Current head a773f3b differs from pull request most recent head 032e3a2. Consider uploading reports for the commit 032e3a2 to get more accurate results

@@            Coverage Diff            @@
##           master   #29313     +/-   ##
=========================================
- Coverage    77.1%    76.5%   -0.6%     
=========================================
  Files          55       54      -1     
  Lines        2934     3121    +187     
  Branches      408      468     +60     
=========================================
+ Hits         2264     2390    +126     
- Misses        529      567     +38     
- Partials      141      164     +23     

Copy link
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.

Nice! You're right that, while true that a Buffer fully implements Uint8Array from Typescript's perspective, folks have been bitten by that toString() behaviour before.

web3.js/yarn.lock Outdated Show resolved Hide resolved
@steveluscher
Copy link
Contributor

Previously: a622198.

@cryptopapi997
Copy link
Contributor Author

I've removed the updated to the lock files - they were not necessary for the fix itself, but were necessary for part of the CI to run which complained about outdated lock files

@steveluscher
Copy link
Contributor

Thanks for the fix!

See you in a couple of weeks, when this breaks something we couldn't predict.

@steveluscher steveluscher merged commit 0b47906 into solana-labs:master Dec 20, 2022
@cryptopapi997 cryptopapi997 deleted the patch-2 branch December 20, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution web3.js Related to the JavaScript client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants