fix: toHex not supporting Uint8Array #6958
Conversation
Signed-off-by: Marin Petrunic <marin.petrunic@gmail.com>
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: d781ddc | Previous: 6c075db | Ratio |
|---|---|---|---|
processingTx |
9360 ops/sec (±3.90%) |
9301 ops/sec (±4.81%) |
0.99 |
processingContractDeploy |
40647 ops/sec (±6.12%) |
39129 ops/sec (±7.62%) |
0.96 |
processingContractMethodSend |
19181 ops/sec (±8.72%) |
19443 ops/sec (±5.19%) |
1.01 |
processingContractMethodCall |
40264 ops/sec (±6.36%) |
38971 ops/sec (±6.34%) |
0.97 |
abiEncode |
42691 ops/sec (±8.40%) |
44252 ops/sec (±6.92%) |
1.04 |
abiDecode |
30239 ops/sec (±6.41%) |
30419 ops/sec (±8.89%) |
1.01 |
sign |
1557 ops/sec (±3.87%) |
1656 ops/sec (±4.08%) |
1.06 |
verify |
382 ops/sec (±0.61%) |
373 ops/sec (±0.78%) |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
| @@ -362,6 +362,10 @@ export const toHex = ( | |||
| return returnType ? 'bigint' : numberToHex(value); | |||
| } | |||
|
|
|||
| if(value instanceof Uint8Array) { | |||
There was a problem hiding this comment.
Could you please use isUint8Array from web3-validator because instanceof is not reliable at some rare edge cases.
| if(value instanceof Uint8Array) { | |
| if( isUint8Array(value) ) { |
Thanks,
There was a problem hiding this comment.
isUint8Array implementation is incorrect as it returns false on buffer even though buffer is uint8array.
In what cases does instanceof not work?
There was a problem hiding this comment.
isUint8Array checks for data.constructor.name === 'Uint8Array' in addition to checking the instanceof. So it is more inclusive than instanceof.
The edge case were instanceof fails was reported at our library at: #6393. And it was also reported to be unreliable at one of our dependencies, where they applied our suggestion (paulmillr/noble-hashes#25 (comment)) to fix it.
There was a problem hiding this comment.
🤦🏻♂️
--rant
Usually when people complain that `x instanceOf Uint8Array`, you are supposed to close the issue and tell them not to use a shitty engine that modifies globals (that also means not using jest+jsdom when you are developing a cross-platform library). 😅
I'm really worried how everyone is ok with libraries and engines like jest/jsdome modifying globals and everybody else is applying hacks to make it work instead of urging people to not use projects like that
-- end rant
We cannot pretend Node.js doesn't exist and ignore Buffers so added commit which add support for Buffer in isUint8Array function and web3-validator.
There was a problem hiding this comment.
Thanks for the modifications. It looks good now that is not solely depending on instanceof.
Thanks also for using the same data?.constructor?.name for Buffers.
A note and a question on the side: It would never be the intention to fix for jest/jsdom because nobody cares about it. And in the issue that was open at our library (#6393) the guy stated "I have a Hyperledger Besu blockchain working on my VM with 4 nodes, built with Qorum Quickstart". And the guy stated that he did not face the issue when he used ethers.js. Do you think that the issue that the guy reported was with jest/jsdom but falsely reported and we should had closed the issue stating "not to use a shitty engine that modifies globals"?
There was a problem hiding this comment.
Hard to say. My assumption is that Besu is using some shitty js engine to execute his javascript script so he should've open issue on besu repo saying that their js execution illegally modifies globals.
I would call bullshit that it works with ethers because: https://github.com/ethers-io/ethers.js/blob/ad5f1c5fc7b73cfb20d9012b669e9dcb9122d244/src.ts/utils/data.ts#L29 😂 😂
There was a problem hiding this comment.
Thanks @mpetrunic , And thanks for looking ethers code.
However, just for clarification, using instanceof is not reliable does not seem to be because of someone modifies globals. But because Uint8Array is "serializable, but not transferable" (#6393 (comment)).
And following the provided links in the issues leads to this article that mentions one of the cases for the failure of using instanceof when the guy is working on a browser extension. This is another example of the edge case that comes because of the nature of Uint8Array being "not transferable" as mozilla documentation stated.
There was a problem hiding this comment.
But doesn't that just mean that somebody illegally transferred "non-transferable" Uint8Array and then it was surprised that intanceof didn't work. Solution would then be to transfer underlying ArrayBuffer and on destination create new Uint8Array(arrayBuffer...) after which instanceof would work?
There was a problem hiding this comment.
But for jsdom: jsdom/jsdom#1769
(for what it's worth happy-dom doesn't have those issues so if you are using vitest it's better to switch)
Signed-off-by: Marin Petrunic <marin.petrunic@gmail.com>
Description
Please include a summary of the changes and be sure to follow our Contribution Guidelines.
Fixes #6957
Type of change
Checklist:
npm run lintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:coverageand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.