test: Add auto-generating basic abi tests for solidity interfaces #2018

nddeluca · 2024-09-06T18:34:55Z

This adds solidity source files and tests that document and assert EVM behavior for solidity function ABI's.

Contracts are added in ABI_BasicTests.sol that define the different function modifiers, special functions cases, etc and the abi_basic.test.ts file asserts all expected behavior dynamically by reading the contract ABI's.

This is currently only testing existing behavior and uses an interface approach to separate the defined ABI from the concrete implementation on chain since these tests will be expanded to use the same interfaces against precompile addresses when run against the kvtool network.

The mocks will be kept as each stateful precompile we add will have an associated mock with the same external behavior (basic input validation, events, etc) so integrating protocols can use them in their test suites without having to always use the kava binary, which is much slower.

Mocks may be extended in the future to support more complex testing or assertions, but for now they are basic and will first be used to verify the behavior of stateful precompiles implementing solidity interfaces matches the expected behavior of the EVM & solidity compiled bytecode.

The empty account and EOA tests are added to increase our e2e testing of EVM behavior through kava & etheremint (hardhat helps us verify we conform to ethereum standards) since these code paths are being modified to support stateful precompiles. They also serve as documentation for reference or learning.

In addition, PR feedback from @boodyvo, @pirtleshell, @downing034 in #2016 is included here.

The equal matcher is a bit clearer and more consistent.

This moves the viem extension to a more clear place and adds viem support for deploy contract on kvtool.

This was changed in a previous commit, but forgot to add to the hardhat config.

This is used by viem and required by the added test helpers as well as typing for using ABI artifacts.

These tests assert and document the behavior of empty and eoa accounts with value when called by external transactions with or without data. In addition, they assert the gas usage is as expected and only depedent on the base transaction cost plus calldata cost since no code is executed.

These tests are added to exercise solidity function ABI's for function and special function calling. This is in preperation for expanding these tests to work against stateful precompiles, ensuring that precompile contracts respect ABI function and special function calling conventions. These tests also serve to increase the test coverage of the Kava EVM and document behavior for future code readers.

These were copied from the empty account tests but usage was removed.

Solhint previously failed due to no contracts, so this adds solhint to CI now that contracts exist. In addition, now that tyepscript uses abitypes from compiled contracts, hardhat compile needs to be run before typescript lint.

A common typo on my part that prettier keeps catching.

This adds a comment to explain that this setting is required for hardhat-viem to not throw an error when sending transactions that will revert to match kava chain behavior.

This removes a comment reference to the tseslint recommended config that is not needed as we are staying with stricter settings.

This adds a linter for smart validation of === vs ==. The linter was verified to fail the comparison in this commit and then verified it passes after the change from == to ===.

This adds noFallthroughCasesInSwitch for switch statements and turns on isolatedModules.

This sets the KAVA_TAG to correctly target the image built locally in CI when running e2e-evm tests for kvtool testnet up and down.

This documents the commands for using act to run the github CI jobs related to e2e-evm locally for lint and e2e tests.

I guess double spaces between sentances are not liked by prettier.

There is a race condition between the typechecking and compiler so we add an explicit compile step here to ensure the race is not hit.

nddeluca · 2024-09-06T19:17:38Z

tests/e2e-evm/test/abi_basic.test.ts

+          it("can not receive plain transfers", async function () {
+            const txData = { to: mockAddress, gas: 25000n, value: 1n };
+
+            const txHash = await walletClient.sendTransaction(txData);


I've been meaning to refactor this duplication out and create much more concise tests but I've deferred for now since I haven't landed on an API I like.

i could see having a helper method with closure over walletClient and publicClient like

async function sendTx(txData) { const hash = await walletClient.sendTransaction(txData); return publicClient.waitForTransactionReceipt({ hash }) }

but does not seem strictly necessary

nddeluca · 2024-09-06T19:18:17Z

tests/e2e-evm/test/abi_basic.test.ts

+        });
+
+        if (fallbackFunction) {
+          it(`can ${fallbackFunction.stateMutability === "payable" ? "" : "not "}receive transfer with data or invalid function selector`, async function () {


I should create a helper here -- isPayable(fallbackFunction) that can be used with an ABI defined function

nddeluca · 2024-09-06T19:18:39Z

tests/e2e-evm/test/abi_basic.test.ts

+          const isPayable = funcDesc.stateMutability === "payable";
+
+          it("can be called", async function () {
+            const txData = { to: mockAddress, data: funcSelector, gas: 25000n };


Default gas should be a constant

nddeluca · 2024-09-06T19:18:52Z

tests/e2e-evm/test/abi_basic.test.ts

+      });
+
+      for (const funcDesc of abi) {
+        if (funcDesc.type !== "function") {


Maybe deserves a comment?

tests/e2e-evm/test/abi_basic.test.ts

nddeluca · 2024-09-06T19:21:20Z

tests/e2e-evm/test/extensions/viem.ts

+  hre.viem.deployContract = (async <CN extends string>(
+    contractName: ContractName<CN>,
+    // eslint-disable-next-line  @typescript-eslint/no-explicit-any
+    constructorArgs: any[] = [],


any is used in the upstream type, so linter ignore was added instead of correcting it

tests/e2e-evm/test/helpers/abi.ts

pirtleshell · 2024-09-06T19:03:43Z

.github/workflows/ci-default.yml

@@ -58,6 +60,9 @@ jobs:
      - name: Install npm dependencies
        run: npm install
        working-directory: tests/e2e-evm
+      - name: Run test suite against hardhat network


Suggested change

- name: Run test suite against hardhat network

- name: Compile contracts and create artifcats

pirtleshell · 2024-09-06T19:15:17Z

tests/e2e-evm/test/helpers/abi.ts

@@ -0,0 +1,21 @@
+import { Abi, AbiFallback, AbiReceive } from "abitype";
+
+export function getAbiFallbackFunction(abi: Abi): AbiFallback | undefined {


ha such a golang guy rocking these for-loops

would recommend a single helper func like (i've converted to functional, keep your for loops if ya want 😄)

export const getAbiFunctionByType = (abi: Abi, funcType: string) => abi.find(f => f.type === funcName)

if you want to have the same funcs as here you can convert them to use that:

const getAbiFallbackFunction = (abi: Abi) => getAbiFunctionByType(abi, "fallback") const getAbiReceiveFunction = (abi: Abi) => getAbiFunctionByType(abi, "receive")

+1, I did a pretty fast extraction of those helpers and should be leveraging these other approaches. The golang habits are strong

pirtleshell · 2024-09-06T19:23:27Z

tests/e2e-evm/test/abi_basic.test.ts

+    // Test no receive + fallback scenarios
+    { interface: "NoopNoReceivePayableFallback", mock: "NoopNoReceivePayableFallbackMock" },
+    { interface: "NoopNoReceiveNonpayableFallback", mock: "NoopNoReceiveNonpayableFallbackMock" },
+  ] as ContractTestCase[];


nit: i'd prefer to declare type this like const testCases: ContractTestCase[] = ... rather than cast with as.
it's a subtle difference, but i think of it like (declaring) "hey, compiler, these are supposed to be ContractTestCases, plz help" vs (casting) "hey, compiler, these ARE ContractTestCase"

I 100% agree and it was the first thing I tried, but typescript was unable to correctly infer contract name type for the mock property with an as ContractTestCase[], so didn't think it was worth duplicating.

Maybe there is another way to hint at the type? Explicitly typing the strings in mock also worked, but it has to be repeated for every element.

types are weird here yeah since they're generated in the artifacts? I tried some wonky thing like this which works for this section but causes some "Unsafe assignment of an error typed value." issues below when using hre.artifacts.readArtifactSync(tc.interface).abi;

interface ContractTestCase<T extends keyof ArtifactsMap> { interface: T; mock: ArtifactsMap[T]["contractName"]; } // ABI_BasicTests assert ethereum + solidity transaction ABI interactions perform as expected. describe("ABI_BasicTests", function () { const testCases: ContractTestCase<keyof ArtifactsMap>[] = [ // Test function modifiers without receive & fallback { interface: "NoopNoReceiveNoFallback", mock: "NoopNoReceiveNoFallbackMock" }, // Test receive + fallback scenarios { interface: "NoopReceiveNoFallback", mock: "NoopReceiveNoFallbackMock" }, { interface: "NoopReceivePayableFallback", mock: "NoopReceivePayableFallbackMock" }, { interface: "NoopReceiveNonpayableFallback", mock: "NoopReceiveNonpayableFallbackMock" }, // Test no receive + fallback scenarios { interface: "NoopNoReceivePayableFallback", mock: "NoopNoReceivePayableFallbackMock" }, { interface: "NoopNoReceiveNonpayableFallback", mock: "NoopNoReceiveNonpayableFallbackMock" }, ]; ...

heh, potentially hacky workaround that skirts the ContractName type troubles:

interface ContractTestCase { interface: keyof ArtifactsMap; mock: `${keyof ArtifactsMap}Mock`; }

but not a major concern lol

pirtleshell · 2024-09-06T19:29:28Z

tests/e2e-evm/test/abi_basic.test.ts

+      describe("State", function () {
+        it("has code set", async function () {
+          const code = await publicClient.getCode({ address: mockAddress });
+          expect(code).to.not.equal(0);


can you confirm that an EOA or never-used address has code 0? i am not 💯 familiar with these operations by i just want to confirm getCode isn't returning a string like "0x" (which also is not equal to the number 0)

You are probably correct here, I believe this is suppose to be checked against undefined -- Viem converts the "0x" response to undefined, I think I confused this assetion with balance or nonce

Could we compare it with an code match from the deployedBytecode in the abi instead of a non-match?

tests/e2e-evm/test/abi_basic.test.ts

pirtleshell · 2024-09-06T19:33:40Z

tests/e2e-evm/test/abi_basic.test.ts

+
+            const txHash = await walletClient.sendTransaction(txData);
+            const txReceipt = await publicClient.waitForTransactionReceipt({ hash: txHash });
+            expect(txReceipt.status).to.equal(isPayable ? "success" : "reverted");


i'd imagine "success" and "reverted" are constants in viem we could use. or create our own. would prevent potential test typos going forward

Good idea, especially since there is no typecheck on the status. I'll create constants since https://github.com/wevm/viem/blob/main/src/utils/formatters/transactionReceipt.ts#L23 are typed string constants that are not exported individually.

At some point I plan to refactor these things to custom chai matchers that will reduce a lot the boilerplate and make the tests much more readable.

drklee3 · 2024-09-06T20:15:13Z

.github/workflows/ci-lint.yml

+      - name: Run solhint
+        run: npm run solhint
+        working-directory: tests/e2e-evm
+      - name: Compile contracts and create artifcats


artif🐱🐱🐱🐱

Suggested change

- name: Compile contracts and create artifcats

- name: Compile contracts and create artifacts

drklee3 · 2024-09-06T20:41:32Z

tests/e2e-evm/test/abi_basic.test.ts

+    // Test no receive + fallback scenarios
+    { interface: "NoopNoReceivePayableFallback", mock: "NoopNoReceivePayableFallbackMock" },
+    { interface: "NoopNoReceiveNonpayableFallback", mock: "NoopNoReceiveNonpayableFallbackMock" },
+  ] as ContractTestCase[];


types are weird here yeah since they're generated in the artifacts? I tried some wonky thing like this which works for this section but causes some "Unsafe assignment of an error typed value." issues below when using hre.artifacts.readArtifactSync(tc.interface).abi;

interface ContractTestCase<T extends keyof ArtifactsMap> { interface: T; mock: ArtifactsMap[T]["contractName"]; } // ABI_BasicTests assert ethereum + solidity transaction ABI interactions perform as expected. describe("ABI_BasicTests", function () { const testCases: ContractTestCase<keyof ArtifactsMap>[] = [ // Test function modifiers without receive & fallback { interface: "NoopNoReceiveNoFallback", mock: "NoopNoReceiveNoFallbackMock" }, // Test receive + fallback scenarios { interface: "NoopReceiveNoFallback", mock: "NoopReceiveNoFallbackMock" }, { interface: "NoopReceivePayableFallback", mock: "NoopReceivePayableFallbackMock" }, { interface: "NoopReceiveNonpayableFallback", mock: "NoopReceiveNonpayableFallbackMock" }, // Test no receive + fallback scenarios { interface: "NoopNoReceivePayableFallback", mock: "NoopNoReceivePayableFallbackMock" }, { interface: "NoopNoReceiveNonpayableFallback", mock: "NoopNoReceiveNonpayableFallbackMock" }, ]; ...

drklee3 · 2024-09-06T20:43:32Z

tests/e2e-evm/test/abi_basic.test.ts

+      describe("State", function () {
+        it("has code set", async function () {
+          const code = await publicClient.getCode({ address: mockAddress });
+          expect(code).to.not.equal(0);


Could we compare it with an code match from the deployedBytecode in the abi instead of a non-match?

pirtleshell

great test additions to endure we don't regress / alter fundamental evm functionality & assumptions 👍

tests/e2e-evm/test/abi_basic.test.ts

pirtleshell · 2024-09-06T19:59:53Z

tests/e2e-evm/test/abi_basic.test.ts

+          it("can not receive plain transfers", async function () {
+            const txData = { to: mockAddress, gas: 25000n, value: 1n };
+
+            const txHash = await walletClient.sendTransaction(txData);


i could see having a helper method with closure over walletClient and publicClient like

async function sendTx(txData) { const hash = await walletClient.sendTransaction(txData); return publicClient.waitForTransactionReceipt({ hash }) }

but does not seem strictly necessary

pirtleshell · 2024-09-06T20:44:24Z

tests/e2e-evm/test/abi_basic.test.ts

+    // Test no receive + fallback scenarios
+    { interface: "NoopNoReceivePayableFallback", mock: "NoopNoReceivePayableFallbackMock" },
+    { interface: "NoopNoReceiveNonpayableFallback", mock: "NoopNoReceiveNonpayableFallbackMock" },
+  ] as ContractTestCase[];


heh, potentially hacky workaround that skirts the ContractName type troubles:

interface ContractTestCase { interface: keyof ArtifactsMap; mock: `${keyof ArtifactsMap}Mock`; }

but not a major concern lol

This adds a test that creates an account via a transaction with non-zero value. We test that the account has a balance (is created) and that the gas charged is the 21000 instrinsic.

nddeluca added 15 commits September 6, 2024 18:23

chore: Prefer equal to eq matcher

fee4499

The equal matcher is a bit clearer and more consistent.

chore: Rename viem extension and add deploy

01f7eb6

This moves the viem extension to a more clear place and adds viem support for deploy contract on kvtool.

chore: Fix the viem extensions path

13f5272

This was changed in a previous commit, but forgot to add to the hardhat config.

chore: Add abitype explicit dependency

28291a8

This is used by viem and required by the added test helpers as well as typing for using ABI artifacts.

chore: Remove unused imports

5b01adb

These were copied from the empty account tests but usage was removed.

chore: Add solhint to CI and fix CI lint

bad97d1

Solhint previously failed due to no contracts, so this adds solhint to CI now that contracts exist. In addition, now that tyepscript uses abitypes from compiled contracts, hardhat compile needs to be run before typescript lint.

chore: format fixes for function parethesis

9762334

A common typo on my part that prettier keeps catching.

chore: Explain throwOnTransactionFailures setting

7bef5a4

This adds a comment to explain that this setting is required for hardhat-viem to not throw an error when sending transactions that will revert to match kava chain behavior.

chore: Remove outdated comment

abfac03

This removes a comment reference to the tseslint recommended config that is not needed as we are staying with stricter settings.

lint: Add eqeqeq smart validation and fix ==

6d6ba28

This adds a linter for smart validation of === vs ==. The linter was verified to fail the comparison in this commit and then verified it passes after the change from == to ===.

chore: Add stricter settings for tsconfig

578df9b

This adds noFallthroughCasesInSwitch for switch statements and turns on isolatedModules.

ci: Target the local build image for e2e-evm

5600f4d

This sets the KAVA_TAG to correctly target the image built locally in CI when running e2e-evm tests for kvtool testnet up and down.

doc: Add act cmds for running e2e-evm CI jobs

6d7a83a

This documents the commands for using act to run the github CI jobs related to e2e-evm locally for lint and e2e tests.

nddeluca requested review from rhuairahrighairidh, karzak, pirtleshell, drklee3, DracoLi, evgeniy-scherbina, sesheffield, boodyvo and lbayas as code owners September 6, 2024 18:34

nddeluca added 2 commits September 6, 2024 18:35

lint: Fix readme prettier formatting

92f525d

I guess double spaces between sentances are not liked by prettier.

ci: Add explicit compile step to fix race cond

a511e03

There is a race condition between the typechecking and compiler so we add an explicit compile step here to ensure the race is not hit.

nddeluca commented Sep 6, 2024

View reviewed changes

tests/e2e-evm/test/abi_basic.test.ts Show resolved Hide resolved

nddeluca commented Sep 6, 2024

View reviewed changes

tests/e2e-evm/test/helpers/abi.ts Show resolved Hide resolved

pirtleshell reviewed Sep 6, 2024

View reviewed changes

drklee3 reviewed Sep 6, 2024

View reviewed changes

pirtleshell approved these changes Sep 6, 2024

View reviewed changes

test: Add empty account creation transaction

6b03924

This adds a test that creates an account via a transaction with non-zero value. We test that the account has a balance (is created) and that the gas charged is the 21000 instrinsic.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add auto-generating basic abi tests for solidity interfaces #2018

test: Add auto-generating basic abi tests for solidity interfaces #2018

nddeluca commented Sep 6, 2024

nddeluca Sep 6, 2024

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024

nddeluca Sep 6, 2024

nddeluca Sep 6, 2024

nddeluca Sep 6, 2024

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024 •

edited

Loading

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024 •

edited

Loading

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024

drklee3 Sep 6, 2024

pirtleshell Sep 6, 2024

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024 •

edited

Loading

drklee3 Sep 6, 2024

pirtleshell Sep 6, 2024

nddeluca Sep 6, 2024

drklee3 Sep 6, 2024

drklee3 Sep 6, 2024

drklee3 Sep 6, 2024

pirtleshell left a comment

pirtleshell Sep 6, 2024

pirtleshell Sep 6, 2024

	- name: Run test suite against hardhat network
	- name: Compile contracts and create artifcats

		@@ -0,0 +1,21 @@
		import { Abi, AbiFallback, AbiReceive } from "abitype";

		export function getAbiFallbackFunction(abi: Abi): AbiFallback \| undefined {

test: Add auto-generating basic abi tests for solidity interfaces #2018

Are you sure you want to change the base?

test: Add auto-generating basic abi tests for solidity interfaces #2018

Conversation

nddeluca commented Sep 6, 2024

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

nddeluca Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nddeluca Sep 6, 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

nddeluca Sep 6, 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

pirtleshell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nddeluca Sep 6, 2024 •

edited

Loading

nddeluca Sep 6, 2024 •

edited

Loading

nddeluca Sep 6, 2024 •

edited

Loading