feat(util): implement assertIsUint8Array
#2271
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
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 |
|
There's virtually no I ran into this while trying to use the |
|
Can we then instead just add another method Just searched, this method is only used within Util it seems anyhow (doesn't mean we can delete though). |
faustbrian
force-pushed
the
uint-buffer
branch
from
e38c019 to
17e1952
Compare
faustbrian
changed the title
Uint8Array as BufferassertIsUint8Array
faustbrian
force-pushed
the
uint-buffer
branch
from
17e1952 to
93cbb51
Compare
| export const generateAddress = function (from: Buffer, nonce: Buffer): Buffer { | ||
| assertIsBuffer(from) | ||
| assertIsBuffer(nonce) | ||
| export const generateAddress = function (from: Buffer, nonce: Uint8Array): Buffer { |
| @@ -77,7 +77,7 @@ export class Address { | |||
| * @param salt A salt | |||
| * @param initCode The init code of the contract being created | |||
| */ | |||
| static generate2(from: Address, salt: Buffer, initCode: Buffer): Address { | |||
| static generate2(from: Address, salt: Buffer, initCode: Uint8Array): Address { | |||
| @@ -107,8 +106,8 @@ const stripZeros = function (a: any): Buffer | number[] | string { | |||
| * @param a (Buffer) | |||
| * @return (Buffer) | |||
| */ | |||
| export const unpadBuffer = function (a: Buffer): Buffer { | |||
| assertIsBuffer(a) | |||
| export const unpadBuffer = function (a: Uint8Array): Buffer { | |||
There was a problem hiding this comment.
I have again this naming problem here. Additionally I don't think it's a good thing for conversion functions to change the type in between, so here, to then have Uint8Array as input and return a Buffer. This is also hard to deprecate at some point.
I wouldn't want to do too quick decisions on these kind of things, since implications on how we can deal with this later on can be significant.
So I think if we do we should likely go the a bit harder way and also add here parallel unpadUint8Array() and the like functionality and then switch over one-by-one at some point deprecate (in the sense of: write a deprecation note, so not directly remove) the Buffer functions (if we decide that we want that. We can also have both in parallel).
There was a problem hiding this comment.
We can also have both in parallel.
There's no reason to support both because a Buffer is literally Uint8Array, see https://nodejs.org/api/buffer.html. It is also less buggy and more consistent in its behaviour. Also see nodejs/node#41588 (comment) by @paulmillr
The Buffer class is a subclass of JavaScript's Uint8Array class and extends it with methods that cover additional use cases. Node.js APIs accept plain Uint8Arrays wherever Buffers are supported as well.
|
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. |
|
This would only be a breaking change if you change return types from |
|
Thoughts on renaming to |
|
|
|
Note: Buffer.slice is not compatible with Uint8array.slice — first returns original memory, second copies it. |
|
Find it a bit of a pity that this has been closed. Maybe we can revive parts of it at some point. |
No description provided.