rlp: v3 updates from integration #1648

ryanio · 2022-01-18T20:29:35Z

This PR picks updates from #1630 to merge onto master to prep the rlp v3 release.

Then #1630 can be rebased and retargeted to the develop branch to be included in the other packages over time as breaking releases add es2020 support for BigInt.

Two new methods (with tests) added to util to help with Buffer<>UInt8Array conversions: arrToBufArr and bufArrToArr, with usage examples added to the rlp changelog.

ryanio · 2022-01-18T20:31:36Z

packages/rlp/src/index.ts

-export type Input = string | number | bigint | Uint8Array | List | null | undefined
+export type Input = string | number | bigint | Uint8Array | Array<Input> | null | undefined

-// Use interface extension instead of type alias to
-// make circular declaration possible.
-export interface List extends Array<Input> {}
+export type NestedUint8Array = Array<Uint8Array | NestedUint8Array>

 export interface Decoded {
  data: Uint8Array | NestedUint8Array
  remainder: Uint8Array
 }

-export interface NestedUint8Array extends Array<Uint8Array | NestedUint8Array> {}
-


I didn't see any type issues changing to circular declaration here, so perhaps TypeScript added support for this from when the comment was originally written.

ryanio · 2022-01-18T20:33:28Z

packages/util/src/bytes.ts

+export function arrToBufArr(arr: Uint8Array | NestedUint8Array): Buffer | NestedBufferArray
+export function arrToBufArr(arr: Uint8Array | NestedUint8Array): Buffer | NestedBufferArray {


I'm not quite sure why I needed to do this final line twice, I thought I was getting pretty good at understanding these overloads too hehe, maybe a TypeScript wiz can help inform me, but I believe I set it up correctly.

Actually, instead of doing this, it would be much cooler if the return type was the same shape array as the input type but Buffers replaced with Uint8Arrays. That would probably require much less casting afterward. I'm not sure if this can be accomplished with generics, but I'll read the documentation and try to see.

codecov · 2022-01-18T20:34:18Z

Codecov Report

Merging #1648 (6d76cdc) into master (eb5bb56) will decrease coverage by 0.00%.
The diff coverage is 90.90%.

Flag	Coverage Δ
block	`85.01% <ø> (ø)`
blockchain	`83.28% <ø> (ø)`
client	`71.27% <ø> (ø)`
common	`93.89% <ø> (ø)`
devp2p	`82.63% <ø> (-0.20%)`	⬇️
ethash	`90.76% <ø> (ø)`
rlp	`90.55% <100.00%> (+1.48%)`	⬆️
trie	`86.18% <ø> (ø)`
tx	`89.94% <ø> (ø)`
util	`92.62% <87.50%> (-0.08%)`	⬇️
vm	`81.27% <ø> (ø)`

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

packages/util/README.md

holgerd77

Hmm, can you please test this new typing of the conversion methods on the v3 PR on #1630? I just injected the typings in there and get a lot of "No overload matches this call." errors already in the Util library when trying to compile.

Maybe these all-catching conversion methods aren't so practical when it comes to typing and one should rather consider 1-to-1 methods with a single input and output? 🤔

Otherwise one would need to always re-cast when these methods get applied.

I guess in most (all?) cases the structure of the input is clear, e.g. if something is a simple Buffer or a Buffer array?

holgerd77 · 2022-01-19T10:22:47Z

packages/rlp/CHANGELOG.md

+const encodedAsBuffer = Buffer.from(encoded)
+const decoded: Uint8Array[] = RLP.decode(encoded)
+const decodedAsBuffers = arrToBufArr(decoded)
+```


Just a note: when we release v3 we should add this to the README to for greater visibility (alternative we can also already add now with a "(not yet released)" addition after "v3" and then remove on v3 release.

sounds good, I already updated the README but can add a section for Buffer compatibility since the topic isn't directly discussed there.

Hmm, can you please test this new typing of the conversion methods on the v3 PR on #1630? I just injected the typings in there and get a lot of "No overload matches this call." errors already in the Util library when trying to compile.

Just tried it out, this was the one place that error'd out: (this is because the version on that branch uses any so it basically skips type checks)

Type 'Decoded' is missing the following properties from type '(Uint8Array | NestedUint8Array)[]': length, pop, push, concat, and 32 more.
const values = arrToBufArr(RLP.decode(Uint8Array.from(serialized)))

This is because we don't know what type RLP.decode outputs, so this can be fixed with:

const values = arrToBufArr(RLP.decode(Uint8Array.from(serialized)) as Uint8Array[])

then we have to cast as Buffer[] for fromValuesArray(values: Buffer[]) because NestedBufferArray !== Buffer[]

which I think is okay. So:

const values = arrToBufArr(RLP.decode(Uint8Array.from(serialized)) as Uint8Array[]) as Buffer[]

a little ugly but better than using any?

My overload typings above just help if you pass in a plain non-array Uint8Array or Buffer you get the non-array version back, so there should be less need for manual overrides in those cases to cast.

have added a Buffer compatibility section to the readme here: https://github.com/ethereumjs/ethereumjs-monorepo/pull/1648/files#diff-68eff8537817899e4ff13ddb30d65da860ae9de825c4426c5551b3bf5ede53a6R35 let me know if it looks good

holgerd77 · 2022-01-19T10:24:05Z

packages/rlp/src/index.ts


-// Use interface extension instead of type alias to
-// make circular declaration possible.
-export interface List extends Array<Input> {}


Not sure if this removal is worth the risk of breaking something on this release, in case someone has imported and used? Maybe we just keep and remove along v3?

Well, v3 is a breaking release so the types are different and not backward compatible

But this is not for v3 but for v2.

So if you remove an existing interface you might break something for some people? 🤔

Or are we somewhat talking past each other here? We can also jump on a quick call later my, earlier your day, mostly available. 🙂

holgerd77 · 2022-01-19T10:25:00Z

packages/rlp/src/index.ts


 export interface Decoded {
  data: Uint8Array | NestedUint8Array
  remainder: Uint8Array
 }

-export interface NestedUint8Array extends Array<Uint8Array | NestedUint8Array> {}


Not sure, might this interface -> type switch cause compatibility problems as well?

This is a new type that didn't exist in v2

I also have this in my current master branch, how can this be then? 🤔

holgerd77

Ok, I am confused here on various stuff, e.g. there is also an ES2020 target in here.

So to recap: My current assumption is that this PR is for an RLP v2.x release which does some preparation for an upcoming v3 release. Is this correct or am I mixing things up?

holgerd77 · 2022-01-20T10:32:03Z

packages/rlp/README.md

+const decodedAsBuffers = arrToBufArr(decoded)
+assert.deepEqual(bufferList, decodedAsBuffers)
+```
+


holgerd77 · 2022-01-20T10:56:57Z

packages/rlp/src/index.ts


-// Use interface extension instead of type alias to
-// make circular declaration possible.
-export interface List extends Array<Input> {}


But this is not for v3 but for v2.

So if you remove an existing interface you might break something for some people? 🤔

Or are we somewhat talking past each other here? We can also jump on a quick call later my, earlier your day, mostly available. 🙂

holgerd77 · 2022-01-20T10:58:04Z

packages/rlp/src/index.ts


 export interface Decoded {
  data: Uint8Array | NestedUint8Array
  remainder: Uint8Array
 }

-export interface NestedUint8Array extends Array<Uint8Array | NestedUint8Array> {}


I also have this in my current master branch, how can this be then? 🤔

holgerd77 · 2022-01-20T11:12:13Z

Update: Ah, now I'll slowly get my misconception.

We do have RLP v3 already on master.

Wow.

That was a long one. 🙄 😛

Hmm, I find this a pretty much unlucky decision though with the scope of BigInt integration not really decided. Cannot really recall why we had been so confident on this? 🤔

I would really very much suggest that we cherry-pick the RLP v3 commits out to a separate branch and then do the releases from there and revert on master for now?

paulmillr · 2022-01-24T14:34:40Z

Any updates @ryanio ?

holgerd77

LGTM

* rlp updates * util: add arrToBufArr and bufArrToArr and tests * util updates, add deprecation notice for re-exports * rlp tsconfig: include rootDir for composite option * remove util exports deprecation notices * rlp: add readme note for buffer compatibility * undo capitalization

holgerd77 · 2022-01-25T10:43:12Z

Sorry, don't want to delay here that much further, but maybe two questions:

@paulmillr a real browser build - as stated as some goal in your initial PR Add browser support, remove dependencies rlp#90 with "This would allow to support browsers out-of-box." is not yet included in this release. So this would still need bunding/packaging or the like. Does this still serve your (and others?) needs or should we add a more direct-to-use dedicated browser build? 🤔
@ryanio @paulmillr Since monorepo: esm support #1654 (ESM build) has progressed that far I wonder if we should integrate here (and: eventually in v2) as well?

paulmillr · 2022-01-25T11:51:39Z

@holgerd77 this is good enough for browsers. The main thing was "0 deps", and "no buffers".

As for ESM, could I suggest to not add it to v2? People will upgrade to v3 sooner in this case 😄

holgerd77 · 2022-01-25T12:04:18Z

@holgerd77 this is good enough for browsers. The main thing was "0 deps", and "no buffers".

Ok, I'll trust you on this one, but just for me to get a better picture (really do not have that much browser-need feeling TBH, would be nice if we expand a bit on this over time 😬): what then is the main browser advantage here, if you integrate this into a build chain (Webpack, whatever) wouldn't this be polyfilled anyhow?

Is there any usability advantage here? Or are you expecting foremost some performance gains?

As for ESM, could I suggest to not add it to v2? People will upgrade to v3 sooner in this case 😄

Hehe.

I am not sure if you are underestimating build targets though. I would assume there are various lib users out there who are just not ready yet for a BigInt switch, so I assume that the v2 release will still be out there for some time (and that would be an argument to also bring generic improvements). Metamask e.g. has Chrome 66 in it's minimal release target, which is still one off from BigInt introduction (not to speak about other ES2020 features. So I wonder if we should really at least disallow some of the newer language constructs/methods per ESLint? 🤔).

paulmillr · 2022-01-25T12:29:19Z

what then is the main browser advantage here, if you integrate this into a build chain (Webpack, whatever) wouldn't this be polyfilled anyhow?

That doesn't always happen, not all bundlers auto-shim buffers. My main goal, as usual, is supply chain auditability - lack of bn.js makes the code much easier to reason about.

As for MM not supporting bigints - well, they'll need to bump their build target from the version which has less than 0.7% of global usage. Otherwise, they won't be able to use our new js-ethereum-cryptography.

Old appe and old build targets can continue to use rlp v2. No one shuts them down, after all. Just want to make transition to bigints faster, because I know how hard it can be to ask folks to upgrade.

holgerd77 · 2022-01-25T16:33:53Z

Ok, I've now published the new version v3.0.0 on npm: https://www.npmjs.com/package/rlp

We'll keep the release a bit under radar until things are a bit tested and there is some first feedback.

* VM: Adopted Backwards-compatible Dynamic Gas Cost Refactoring (#1553) * VM: report dynamic gas values in `fee` field of `step` event (#1364) * vm: codes: add dynamicGas property vm: codes: make codes more typesafe * vm: create no-op dynamic gas handlers * vm: first batch of dynamic gas up to 0x3f * vm: add other opcodes to gas map vm: change step event fee type from number to BN vm: deduct dynamic gas vm: fix stack peek vm: do not add gasLimit to gas cost for CREATE * vm: move errors to gas map * vm: fix memory dynamic gas bug * vm: fix gas bugs caused by not considering base fee * vm: fix message call gas related bugs, clone current gas left * add typedoc for peek use underscore for unused peek params (and fix eslint config) format some comment newlines for readability switch from require to import for exceptions * simplify the 2929 state manager castings in runTx * add changelog entry * vm: add EIP1283 tests * vm: split non-eip2929 and eip2929 gas costs * vm: fix gas costs * vm: add early-hardfork coverage * vm: clarify pre-Constantinople SSTORE gas vm: clarify EIP-150 comment * run coverage for all state and blockchain tests, remove redundant istanbul run * vm: fix CALLCODE gas vm: explicitly clone gasLimit for maxCallGas * vm: remove TODO in interpreter * update defaultCost to BN, cast 2929 statemanager to simplify use syntax * use underscore for unused variables, simplify types since they can be inferred * vm: fix browser tests + fix rebase * VM: moved dynamic fee to dedicated dynamicFee field in step event and retained fee behavior and type for backwards compatibility * VM: aligned InterpreterStep and step event object property documentation, completed missing step event properties * VM: test fix * vm: fix hardhat e2e tests * vm: fix MSTORE opcodes * vm: added dynamicGas property to push0 (EIP-3855) opcode * hardhat e2e: add temporary workaround for skipping tests with inconsistent memory field * nit style: use underscore instead of comment out unused variable Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com> * ci: bump karma-typescript version (#1631) * bump karma-typescript to new release * bump libp2p-bootstrap * update package-lock * VM: Jump dest analysis enhancements (#1629) * Change valid jumps to single array * Switch array to Uint8Array * use correct opt code * Check for jumpsub * Address feedback * Address fixes * More efficiency * Adjust if/else logic for efficiency * Move comments Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: Ryan Ghods <ryan@ryanio.com> * rlp: add to root readme (#1642) * add rlp to root readme * add carryforward for rlp flag since it is not always tested on every build unless its paths are modified * Client: Add tests to verify shutdown (#1641) * Add tests to verify client shutdown * Add libp2p test * Address feedback * most libp2p into separate file * VM: Add comparison testing between branchs for State Test Runner (#1634) * Add diff tester and performance * update script * Simplify script * Readme updates * move start * Update stashing logic in script * tx: input value testing (#1620) * tx: helper methods to generate tx values * tx: helper methods to generate tx values * chore: scaffold test and add extra to values * chore: add missing spread operator * chore: single value for each tx key * tx: add generateCombinations method and legacy + eip 1559 test cases * chore: remove console log * chore: use london hardfork for eip1559 * tx: refactor generateCombinations interface and properly compare hash * tx: deterministically randomly sample 1000 elements from array for testing * chore: remove v * chore: remove todo * chore: clearer variable declarations and types * tx: fix and simplify random sampling * tx: display tx hash in testing output * tx: convert buffer to hex for output log * ci: fix node-versions run for node <16 (#1653) * re-add updating to npm v7 for node versions <16 * only upgrade npm for node v <16 * fix bin/rlp js: node 12 doesn't support ES11 which added support for nullish coalescing operator (??) so we'll use ternary here alternatively we could write this file in TS and compile to e.g. dist/bin/rlp (like we do in the client bin), but maybe if the file gets more complicated, in its current state i don't think it's so neccessary * use same errorMsg format for JSON.parse, remove unneeded extra Uint8Array.from (already is uint8array) * rlp: v3 updates from integration (#1648) * rlp updates * util: add arrToBufArr and bufArrToArr and tests * util updates, add deprecation notice for re-exports * rlp tsconfig: include rootDir for composite option * remove util exports deprecation notices * rlp: add readme note for buffer compatibility * undo capitalization * Devp2p, Client: Fix duplicated debug messages (#1643) * devp2p: update debug package * blockchain: update debug package * client: update debug package * vm: update debug package * devp2p/dpt fix for #1485 * devp2p/eth: Fix for 485 * devp2p: Fix for #1485: add base debug * devp2p/dpt #1485: change to debug.extend() * devp2p:dpt:server change to degub.extend() * devp2p:eth change to debug.extend() * devp2p/les: change to debug.extend() * devp2p:rlpx: change to debug.extend() * rlps: change to debug.extend() * Update debug to use IP as first tag * vm, client: removed ProofStateManager interface, added optional getProof, verifyProof methods to StateManager interface (#1660) * Monorepo (& Tx, VM): Examples scripts in CI (#1658) * chore(examples): examples added to ci * chore(examples-ci): remove script from VM (for now) & rename examples workflow file * chore(ci): new script formwatted with prettier & example workflow changes to run with non-test branches Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com> Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com> Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com> Co-authored-by: ScottyPoi <66335769+ScottyPoi@users.noreply.github.com> Co-authored-by: Cesar Brazon <cesarbrazon10@gmail.com>

ryanio added PR state: needs review package: util package: rlp labels Jan 18, 2022

ryanio commented Jan 18, 2022

View reviewed changes

holgerd77 reviewed Jan 19, 2022

View reviewed changes

packages/util/README.md Outdated Show resolved Hide resolved

holgerd77 requested changes Jan 19, 2022

View reviewed changes

ryanio added 6 commits January 19, 2022 10:32

rlp updates

d69d1ce

util: add arrToBufArr and bufArrToArr and tests

042e4ca

util updates, add deprecation notice for re-exports

5107135

rlp tsconfig: include rootDir for composite option

8b69ae8

remove util exports deprecation notices

d0babee

rlp: add readme note for buffer compatibility

47cad6b

ryanio force-pushed the rlp-v3-updates branch from bf71865 to 47cad6b Compare January 19, 2022 18:32

ryanio added 2 commits January 19, 2022 10:34

undo capitalization

7e76fd9

Merge branch 'master' into rlp-v3-updates

6d76cdc

ryanio requested a review from holgerd77 January 19, 2022 18:41

holgerd77 reviewed Jan 20, 2022

View reviewed changes

holgerd77 approved these changes Jan 24, 2022

View reviewed changes

ryanio merged commit 56fbb0b into master Jan 24, 2022

ryanio deleted the rlp-v3-updates branch January 24, 2022 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rlp: v3 updates from integration #1648

rlp: v3 updates from integration #1648

ryanio commented Jan 18, 2022

ryanio Jan 18, 2022

ryanio Jan 18, 2022

ryanio Jan 19, 2022

codecov bot commented Jan 18, 2022 •

edited

Loading

holgerd77 left a comment

holgerd77 Jan 19, 2022

ryanio Jan 19, 2022

ryanio Jan 19, 2022

ryanio Jan 19, 2022

holgerd77 Jan 19, 2022

ryanio Jan 19, 2022

holgerd77 Jan 20, 2022

holgerd77 Jan 19, 2022

ryanio Jan 19, 2022

holgerd77 Jan 20, 2022

holgerd77 left a comment

holgerd77 Jan 20, 2022

holgerd77 Jan 20, 2022

holgerd77 Jan 20, 2022

holgerd77 commented Jan 20, 2022

paulmillr commented Jan 24, 2022

holgerd77 left a comment

holgerd77 commented Jan 25, 2022

paulmillr commented Jan 25, 2022

holgerd77 commented Jan 25, 2022

paulmillr commented Jan 25, 2022

holgerd77 commented Jan 25, 2022 •

edited

Loading

		export function arrToBufArr(arr: Uint8Array \| NestedUint8Array): Buffer \| NestedBufferArray
		export function arrToBufArr(arr: Uint8Array \| NestedUint8Array): Buffer \| NestedBufferArray {

rlp: v3 updates from integration #1648

rlp: v3 updates from integration #1648

Conversation

ryanio commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 18, 2022 • edited Loading

Codecov Report

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Jan 20, 2022

paulmillr commented Jan 24, 2022

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Jan 25, 2022

paulmillr commented Jan 25, 2022

holgerd77 commented Jan 25, 2022

paulmillr commented Jan 25, 2022

holgerd77 commented Jan 25, 2022 • edited Loading

codecov bot commented Jan 18, 2022 •

edited

Loading

holgerd77 commented Jan 25, 2022 •

edited

Loading