Add backwards-compatibility test fixture and run in CI for tx

[cgewecke]

#1111

a test setup should be integrated so that some/all/selected API tests can be run with a different dependency version, e.g. by providing a different package.json file or something.

This PR's strategy is to swap an npm published version of a sub-dependency (ex: tx) into the monorepo, replacing the current branch's version. Then everything is re-linked and vm API tests are run with the new version.

Have added a shell script that can execute this process generically for any sub-dependency/target pair. It restores the monorepo to its original state after executing so in principle it can be used locally but have added an env variable safety check to dissuade people from doing this accidentally.

It should also be possible to execute repeatedly in CI in the vm-backwards-compatibility job for different subpackages (mpt, etc)

Usage:

$ ./scripts/check-compatibility.sh [options]

> OPTIONS (all are required)
> -t <test>       monorepo package whose tests we'd like to run (ex: vm)
> -d <dependency> monorepo package dependency we want to swap out (ex: tx)
> -n <npm>        npm package name to swap in (ex: @ethereumjs/tx)
> -v <version>    npm package version to swap in (ex: 3.0.2)
> -s <script>     script to "npm run" to execute the tests

The script flow for this case is

• Move packages/tx to a safe place (stash/)
• Create a tx folder in the project root, cd in and npm install @ethereumjs/tx there
• Move the contents of tx/node_modules/@ethereumjs/tx out 2 levels so that the folder looks like

tx/
  dist/
  dist.browser/
  node_modules/
  package.json
  etc...

• Move tx/ into packages/
• Re-link everything
• Run vm tests
• Unspool all the above

[codecov]

Codecov Report

Merging #1118 (a95fdfd) into master (5620105) will increase coverage by 0.11%.
The diff coverage is n/a.

Flag	Coverage Δ
block	81.78% <ø> (-0.04%)	⬇️
blockchain	84.13% <ø> (-0.07%)	⬇️
client	87.04% <ø> (-0.01%)	⬇️
common	87.87% <ø> (+0.63%)	⬆️
devp2p	84.71% <ø> (+0.36%)	⬆️
ethash	82.08% <ø> (ø)
tx	85.50% <ø> (ø)
vm	82.00% <ø> (ø)

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

[cgewecke]

In this CI run a "verification" log line is injected into the npm sourced package's Transaction constructor (before swapping) to demonstrate that the substitution takes place as expected.

[holgerd77]

Great, generally stay in close sync with @jochem-brouwer on this, still not sure if this will be necessary on this round or if #1048 will stay generally backwards-compatible, but independently from that it's great to have this, we'll see if it comes to application directly.

One question I have is how to solve the TypeScript signature typing for txs elegantly so that runTx(this: VM, opts: RunTxOpts) don't complain on typing when passed in a v3 Tx, not sure about the design here, just to mention that this needs to be thought along as well.

[cgewecke]

Rebased

[cgewecke]

This test sort of works...

Maybe this line should get an explicit cast to EIP2930Transaction?

ethereumjs-monorepo/packages/vm/lib/runTx.ts

Line 102 in 5620105

if (opts.tx.transactionType === 1 && this._common.isActivatedEIP(2929)) {

-  if (opts.tx.transactionType === 1 && this._common.isActivatedEIP(2929)) {
+  if ((opts.tx as EIP2930Transaction).transactionType === 1 && this._common.isActivatedEIP(2929)) {

A more correct version of this test might look like the setup in #1134 and a simple mock project whose dependencies are:

@ethereumjs/tx: 3.0.2
@ethereumjs/vm: master
  |- @ethereumjs/tx: master

...with tests similar to vm/test/api/runTx.ts but not actually run inside the vm package so the types imports are not an issue.

Seems like the implementation is pretty non-breaking to me though...

[holgerd77]

This test sort of works...

Maybe this line should get an explicit cast to EIP2930Transaction?

A cast wouldn't work here, since this actually should work without this being an EIP2930Transaction, but a simple property check should do on such occurrences, see comment here https://github.com/ethereumjs/ethereumjs-monorepo/pull/1048/files#r587391549.

[cgewecke]

Am going to close here and pursue my suggestion for a more realistic test above as part of reviewing #1138. This test isn't designed correctly.

[holgerd77]

Since we now already have two use cases for this (testing the tx-related VM methods, testing the Block constructor on passing in tx instances), maybe it is possible to design this in a way not tied too much to a specific package but in a relative generic way which would work for all the packages? 🤔

Not sure, just a thought, if it gets too complicated definitely also viable to copy-paste e.g. some GH actions code from one actions file to another or something.