feat(util): implement `assertIsUint8Array`

[faustbrian]

[codecov[bot]]

Codecov Report

Merging #2271 (93cbb51) into master (1e0de28) will increase coverage by 0.00%. The diff coverage is 100.00%.

Flag	Coverage Δ
block	92.89% <ø> (ø)
blockchain	88.47% <ø> (ø)
client	86.94% <ø> (ø)
common	98.09% <ø> (ø)
devp2p	92.56% <ø> (+0.10%)	:arrow_up:
ethash	∅ <ø> (∅)
evm	79.25% <ø> (ø)
rlp	∅ <ø> (?)
statemanager	88.47% <ø> (ø)
trie	90.32% <ø> (-0.05%)	:arrow_down:
tx	97.98% <ø> (ø)
util	92.11% <100.00%> (-0.21%)	:arrow_down:
vm	85.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

I don't think this is a safe thing to do since one can't anticipate all the use cases here. If this is used for specifically checking for a Buffer on a value and after that Buffer-specific functionality is called things would break.

So my current tendency is to not merge this in.

We can work our way through the libraries and see where we can take in Uint8Array instead of Buffer in a first round for feature functionality methods though without side effects, at least that would be my first-thought suggestion where it would be possible to start on this a bit.

[faustbrian]

There's virtually no Buffer-specific functionality used in this repository (or it's dependencies that rely on Buffer) and you would have to really go out of your way to even find functionality that Buffer offers but Uint8Array doesn't.

I ran into this while trying to use the util package with a Uint8Array instead of Buffer which then threw an exception while the functionality works just fine with Uint8Array (like everything else in this repository)

[holgerd77]

Can we then instead just add another method assertIsUint8Array() and switch to that where it is possible? I guess I just don't like the fact that the function after this change just doesn't do any more what it claims to do "by name".

Just searched, this method is only used within Util it seems anyhow (doesn't mean we can delete though).

[holgerd77]

Generally, do not give too much expectations on having this merged in soon. We have spent months and months on these Buffer vs Uint8Array discussions.

If we want to pick this up again I want to make sure that we have a gentle, consistent and non-breaking strategy (optimally written down as an issue or something) throughout the whole monorepo before we act upon this, especially so close after the breaking releases.

[faustbrian]

This would only be a breaking change if you change return types from Buffer to Uint8Array but if only inputs are changed then TypeScript won't even throw any errors because a Buffer is Uint8Array.

[paulmillr]

Thoughts on renaming to assertIsUi8a?

[faustbrian]

Buffer is basically Buffer extends Uint8Array with some features and buggy behaviours you probably have never heard of or used and never will. Wherever you hint Uint8Array you can pass in Buffer and in 99.9% of cases it will just work unless you some obtuse functionality of Buffer.

See https://nodejs.org/api/buffer.html

[paulmillr]

Note: Buffer.slice is not compatible with Uint8array.slice — first returns original memory, second copies it.

[holgerd77]

Find it a bit of a pity that this has been closed. Maybe we can revive parts of it at some point.