tx: input value testing #1620
tx: input value testing #1620
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
gabrocheleau
requested review from
acolytec3,
holgerd77,
jochem-brouwer and
ryanio
packages/tx/test/inputValue.spec.ts
Outdated
|
|
||
| function getRandomSubarray(array: any[], size: number) { | ||
| const shuffled = array.slice(0), | ||
| seed = 1559 |
There was a problem hiding this comment.
This is probably just me not being a Javascript guru but this syntax...
const shuffled = array.slice(0),
seed = 1
threw me for a loop. Is the comma operator here just applying const to the seed = 1 expression? Reading on mozilla dev docs here didn't make it any more obvious to me - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comma_Operator
There was a problem hiding this comment.
Yeah, it's a shorthand way of declaring multiple variables, but I agree it's not too readable and commonly used, I'll revert to an explicit declaration per line.
packages/tx/test/inputValue.spec.ts
Outdated
| let index: number, | ||
| i = array.length, | ||
| temp: any[] | ||
| while (i--) { |
There was a problem hiding this comment.
Here again, more "I'm not a javascript guru" questions but presumably while (i--) is just shorthand for while (i === 0) { i--} ? I dropped some console logs in the test and that seems to be what's happening but just wanted to confirm.
There was a problem hiding this comment.
It's a shorthand for
while (i > 0) {
i = i - 1;
}
There was a problem hiding this comment.
oh nice didn't know this one. maybe you can also do one declaration per line for the above let variables as well. i wonder if there's a eslint rule to prefer one line per declaration
found it: ^_^ https://eslint.org/docs/rules/one-var-declaration-per-line
There was a problem hiding this comment.
I think we should avoid such abbreviations, we have already two people stumbling upon this here respectively not knowing how this would behave (with me it would be three 😋) and this just makes code less approachable if 50% of developers do not know how to read the code.
(sometimes a bit difficult to draw the lines but I would say in this case it's relatively clear)
There was a problem hiding this comment.
Good point, will update! :)
|
What's the status of this? Can we have this fixed? |
Yes, I have some time tomorrow evening/Saturday and will get that PR finished then. |
There was a problem hiding this comment.
Looks great! I dumped some of the txData values to the console for both legacy and EIP1559 transactions and am seeing all the different data types represented and no issues testing. I also ran it several times just to get a variety of different combos and its working great. Nice work!
packages/tx/test/inputValue.spec.ts
Outdated
| for (const txData of randomSample) { | ||
| const tx = Transaction.fromTxData(txData, { common }) | ||
|
|
||
| st.deepEqual(tx.hash(), expectedHash), 'correct tx hash' |
There was a problem hiding this comment.
This is a fun one (analog below): the test output string is actually outside the function call and is not taken but text output is "should be deeply equivalent". 😅
Totally love this, that even the syntax here seems to work (what kind of syntax is that actually?) Did some tests: 'test', 'toast' actually throws with "Left side of comma operator is unused and has no side effects." but let t = function() {} followed by t(), 'test' actually works and outputs the string. Lol.
Anyhow: maybe you can update! 🙂 Is there a (simple) way to get some of the test data into the output message too? Or is this too complicated respectively get's too verbose with the current code structure? Would be nice to identify single test cases, but also no absolute must.
Otherwise PR looks really great! 👍
There was a problem hiding this comment.
Ah! Nice catch, that syntax is really weird indeed, now fixed!
I've added the tx.hash() to the output message, this at least allows us to differentiate between transaction objects (e.g. the legacy case vs. the eip1559 case). I think it would be too verbose to display the tx args themselves.
There was a problem hiding this comment.
Yeah, that's a good compromise, thanks!
Sure thanks for the review! Will check that out and address later. |
* 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 <[email protected]> Co-authored-by: Ryan Ghods <[email protected]> * 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 <[email protected]> Co-authored-by: Ryan Ghods <[email protected]> * 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 <[email protected]> Co-authored-by: Holger Drewes <[email protected]> Co-authored-by: acolytec3 <[email protected]> Co-authored-by: Gabriel Rocheleau <[email protected]> Co-authored-by: ScottyPoi <[email protected]> Co-authored-by: Cesar Brazon <[email protected]>
Resolves #1610