Add smoketest to e2e tests #261

piersy · 2024-10-18T18:19:45Z

Adds an e2e smoketest that sends value transfer, contract interaction
and contract creation transactions for all of the valid transaction types.

It also verifies that deprecated transactions (celo legacy & cip42) are
not supported.

e2e_test/js-tests/test_viem_smoketest.mjs

karlb · 2024-10-21T06:23:52Z

e2e_test/js-tests/test_viem_smoketest.mjs

+	// This test is failing because the produced transaction is of type cip64.
+	// I guess this is a problem with the viem internals.
+	it.skip("cip42 not supported", async () => {
+		const type = "cip42";
+		await verifyTypedTransactions(type);
+	}).timeout(10_000);


Interesting and I think your explanation is correct. We could try running the same test against Celo L1 to make sure it's not a change in the chain behaviour that is causing this.

@piersy just to get the explanation right - viem with the new l2 alfajores definitions does not seem to know about "cip42" types and instead produces a cip64 transaction, that then fails the assertion?

I would classify this as a viem bug, since it shouldn't even allow the deprecated `cip42' type and fail early on unknown types (at least in my expectation).

yeah, I'd classify it as a viem bug, i will raise it with the dev tooling team

However viem still needs to be able to send cip42 txs to the celo l1, so it does still need that functionality.

Does it? It could just send CIP-64 without missing any features.

Unless people really want to set the gatewayFee and gatewayFeeRecipient! I'll check with the devtooling team to see what the expected approach for viem is.

You can't set those to non-zero values anymore (celo-blockchain will reject the tx otherwise), so nothing is lost.

Ok, was unaware!

@karlb @ezdac I've moved this test into the smoketest_unsupported_txs golang test. Where we can be sure that we are sending an invalid transaction, and therefore ensure that the server correctly rejects it.

e2e_test/js-tests/test_viem_smoketest.mjs

karlb · 2024-10-21T06:32:59Z

e2e_test/js-tests/test_viem_smoketest.mjs

+			assert.fail("Managed to send unsupported legacy tx with fee currency");
+	}).timeout(10_000);
+
+	it.only("legacy create tx with fee currency not supported", async () => {


I'm not sure if it's worth having this test in addition to the previous one. Chances that tx types behave totally differently between send and create are low and we already test plenty of both in other tests with having to repeat them.

Hey @karlb I've removed these tests from the viem test and added them in golang since its easier to construct known unsupported transactions in golang, in the golang version I only test send transactions.

e2e_test/test_smoketest.sh

ezdac

Looks good so far! I added some suggestions and questions regarding unsupported tx types.
This branch requires a rebase against celo10.

e2e_test/js-tests/test_viem_smoketest.mjs

ezdac · 2024-10-21T23:08:06Z

e2e_test/js-tests/test_viem_smoketest.mjs

+	// This test is failing because the produced transaction is of type cip64.
+	// I guess this is a problem with the viem internals.
+	it.skip("cip42 not supported", async () => {
+		const type = "cip42";
+		await verifyTypedTransactions(type);
+	}).timeout(10_000);


@piersy just to get the explanation right - viem with the new l2 alfajores definitions does not seem to know about "cip42" types and instead produces a cip64 transaction, that then fails the assertion?

I would classify this as a viem bug, since it shouldn't even allow the deprecated `cip42' type and fail early on unknown types (at least in my expectation).

e2e_test/js-tests/test_viem_smoketest.mjs

.github/workflows/ci.yaml

karlb · 2024-10-22T13:23:06Z

Why is the CI workflow with the tests not run for the last commits? The workflow file looks fine to me.

piersy · 2024-10-22T13:33:15Z

Why is the CI workflow with the tests not run for the last commits? The workflow file looks fine to me.

Hey @karlb, no idea I only made one change to CI which was to run these tests on alfajores. Maybe because the automatic base change or the conflicts it now has?

Update: The workflow seemed to get run again after rebasing this on celo10, perhaps it was the existence of conflicts between this and celo10 that caused the workflow to not get run.

The smoketest tries all types of transaction for both sends and creates.

Chose a timeout of 16 seconds since the first two tests were taking 8 seconds on Alfajores.

Note that in many cases we were able to remove explicit use of account in transaction creattion because it is "hoisted" in the walletClient. Also removed chain-id and ACC_PRIVKEY as arguments from send_tx.mjs because they are controlled by env vars, which are picked up by the mjs file.

e2e_test/js-tests/test_viem_smoketest.mjs

e2e_test/test_smoketest.sh

Co-authored-by: Karl Bartel <[email protected]>

Due to the way that viem is structured it is a bad idea for us to block transactions based on the supplied tx attributes, since they may be supported by downstream components such as optimism's custom viem functionality. So rather than trying to test unsupported txs via viem, we now test via golang which allows us full control of the sent transaction, thereby ensuring that rejection is happening at the node level. This commit also extends the error message for deprecated transactions to distinguish between celo legacy and plain legacy transactions.

karlb · 2024-10-23T15:55:42Z

e2e_test/smoketest_unsupported_txs/unsupported_txs_test.go

+}
+
+func parsePrivateKey(privateKey string) (*ecdsa.PrivateKey, error) {
+	if len(privateKey) >= 2 && privateKey[0] == '0' && (privateKey[1] == 'x' || privateKey[1] == 'X') {


if len(privateKey) == 66 would also have been an easy way to check for a 0x-prefixed private key.

Adds a new workflow that triggers e2e test runs on alfajores once a day, and also in PRs whenever the e2e tests are updated.

ezdac

Looks great now!

ezdac · 2024-10-24T14:57:34Z

e2e_test/smoketest_unsupported_txs/unsupported_txs_test.go

+	flag.StringVar(&feeCurrency, "fee-currency", "", "address of the fee currency to use")
+}
+
+func TestTxSendingFails(t *testing.T) {


Will this be discovered by a go test ./... and then lack the flags?
If so, maybe for now skip the test if the flags are not present and later we can spawn a local test environment if the flags are not present?

No it has a build flag, so it's only picked up if you specify -tags smoketest

karlb · 2024-11-13T14:37:31Z

Is there any reason for not having merged this yet, or is it just the merge conflicts?

piersy · 2024-11-25T11:43:51Z

Is there any reason for not having merged this yet, or is it just the merge conflicts?

I think it just got superseded by our bad block problems, I've updated it now, so should be able to merge shortly 🤞

Adds an e2e smoketest that sends value transfer, contract interaction and contract creation transactions for all of the valid transaction types. It also verifies that deprecated transactions (celo legacy & cip42) are not supported. Co-authored-by: Karl Bartel <[email protected]>

piersy requested review from karlb and ezdac October 18, 2024 18:19

karlb reviewed Oct 21, 2024

View reviewed changes

Base automatically changed from piersy/run-e2e-tests-on-alfajores to celo10 October 21, 2024 10:58

karlb approved these changes Oct 21, 2024

View reviewed changes

e2e_test/test_smoketest.sh Outdated Show resolved Hide resolved

ezdac requested changes Oct 21, 2024

View reviewed changes

karlb reviewed Oct 22, 2024

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

piersy force-pushed the piersy/smoketest branch from 30c3afa to 7fb90b5 Compare October 22, 2024 13:52

piersy added 13 commits October 22, 2024 15:06

Add smoketest to e2e tests

81beb5b

The smoketest tries all types of transaction for both sends and creates.

Fix erroneous timeout

013cf7e

Remove use of only (left over from local debugging)

54b3a9b

Add a sleep to prevent spamming the server

a65314e

Set smoketest timeout globally

ab7836f

Chose a timeout of 16 seconds since the first two tests were taking 8 seconds on Alfajores.

Add smart contract interactions to smoke test

e89bad1

Use writeContract instead of sendTransaction

c1cfab5

Remove trailing space

34ea9bd

Correct formatting

5158b37

Move assertions next to fetching actions

d8e7235

Parameterise viem smoketests

3955e5f

Fix just enough gas test

1f87012

piersy force-pushed the piersy/smoketest branch from 7fb90b5 to 1f87012 Compare October 22, 2024 14:33

Update skipped test to not reference missing function

418e542

aaronmgdr reviewed Oct 22, 2024

View reviewed changes

e2e_test/js-tests/test_viem_smoketest.mjs Show resolved Hide resolved

karlb reviewed Oct 23, 2024

View reviewed changes

e2e_test/test_smoketest.sh Outdated Show resolved Hide resolved

piersy and others added 3 commits October 23, 2024 13:11

Update e2e_test/test_smoketest.sh

fcf6c8e

Co-authored-by: Karl Bartel <[email protected]>

Don't run golang smoketest run with usual go tests

1960e67

piersy requested review from karlb and ezdac October 23, 2024 15:24

karlb approved these changes Oct 23, 2024

View reviewed changes

piersy force-pushed the piersy/smoketest branch from 83646cc to 8169cda Compare October 23, 2024 16:26

Trigger e2e test runs on alfajores differently

3e50913

Adds a new workflow that triggers e2e test runs on alfajores once a day, and also in PRs whenever the e2e tests are updated.

piersy force-pushed the piersy/smoketest branch from 8169cda to 3e50913 Compare October 23, 2024 16:28

piersy added 8 commits October 23, 2024 17:39

Add debug to see why file is not found

c76e8d1

Ensure tree is installed

9e0ceba

Update before install

d8b70ef

Update forge version and reinstate dot

34e4753

Provide explicit paths to forge

dd60913

Remove debug

ed2024c

Merge branch 'celo10' into piersy/smoketest

00e325e

Merge branch 'celo10' into piersy/smoketest

effa0fe

ezdac approved these changes Oct 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/celo10' into piersy/smoketest

acb90f1

piersy added 2 commits November 25, 2024 12:01

Merge branch 'celo10' into piersy/smoketest

6622671

Merge branch 'celo10' into piersy/smoketest

8a205ae

piersy merged commit 70a7e9b into celo10 Nov 26, 2024
9 checks passed

piersy deleted the piersy/smoketest branch November 26, 2024 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add smoketest to e2e tests #261

Add smoketest to e2e tests #261

piersy commented Oct 18, 2024 •

edited

Loading

karlb Oct 21, 2024

ezdac Oct 21, 2024

piersy Oct 22, 2024

piersy Oct 22, 2024

karlb Oct 22, 2024

piersy Oct 22, 2024

karlb Oct 22, 2024

piersy Oct 22, 2024

piersy Oct 23, 2024

karlb Oct 21, 2024

piersy Oct 23, 2024

ezdac left a comment

ezdac Oct 21, 2024

karlb commented Oct 22, 2024

piersy commented Oct 22, 2024 •

edited

Loading

karlb Oct 23, 2024

ezdac left a comment

ezdac Oct 24, 2024

piersy Oct 24, 2024

karlb commented Nov 13, 2024

piersy commented Nov 25, 2024 •

edited

Loading

Add smoketest to e2e tests #261

Add smoketest to e2e tests #261

Conversation

piersy commented Oct 18, 2024 • edited Loading

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

ezdac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlb commented Oct 22, 2024

piersy commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

ezdac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlb commented Nov 13, 2024

piersy commented Nov 25, 2024 • edited Loading

piersy commented Oct 18, 2024 •

edited

Loading

piersy commented Oct 22, 2024 •

edited

Loading

piersy commented Nov 25, 2024 •

edited

Loading