Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🧪 Test: Add more test coverage to actions #1466

Merged
merged 6 commits into from
Sep 29, 2024
Merged

🧪 Test: Add more test coverage to actions #1466

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea1a139
:test_tube: Test: Add more test coverage to actions
Sep 29, 2024
fa87a5e
:lipstick: Linter
Sep 29, 2024
efb8b71
:rewind: Revert unneded changes
Sep 29, 2024
b14c990
:test_tube: Test: Add a forked test
Sep 29, 2024
bb288ba
:fire: Unused import
Sep 29, 2024
cb6cad5
:recycle: Fix: lint
Sep 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions packages/actions/src/debug/debugTraceCallProcedure.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { createAddress } from '@tevm/address'
import { createTevmNode } from '@tevm/node'
import { numberToHex, parseEther } from '@tevm/utils'
import { describe, expect, it } from 'vitest'
import { debugTraceCallJsonRpcProcedure } from './debugTraceCallProcedure.js'

// TODO this test kinda sucks because it isn't tracing anything but since the logic is mostly in callHandler which is tested it's fine for now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address the TODO comment to improve test quality.

The TODO comment indicates that the current test might not be comprehensive enough. Consider enhancing the test to cover more scenarios or to trace actual operations.

As a follow-up, could you provide more context on why the current test "kinda sucks"? This information would be helpful in determining how to improve the test coverage.


describe('debugTraceCallJsonRpcProcedure', () => {
it('should trace a call and return the expected result', async () => {
const client = createTevmNode()
const procedure = debugTraceCallJsonRpcProcedure(client)

const result = await procedure({
jsonrpc: '2.0',
method: 'debug_traceCall',
params: [
{
to: createAddress('0x1234567890123456789012345678901234567890').toString(),
data: '0x60806040',
value: numberToHex(parseEther('1')),
tracer: 'callTracer',
},
],
id: 1,
})

expect(result).toMatchInlineSnapshot(`
{
"id": 1,
"jsonrpc": "2.0",
"method": "debug_traceCall",
"result": {
"failed": false,
"gas": "0x0",
"returnValue": "0x",
"structLogs": [],
},
}
`)
})
Comment on lines +28 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding explicit assertions alongside the snapshot.

While the inline snapshot is useful for capturing the overall structure of the response, it might be beneficial to add explicit assertions for critical values. This would make the test's intentions clearer and easier to maintain.

Consider adding explicit assertions before the snapshot, like this:

expect(result.result.failed).toBe(false)
expect(result.result.gas).toBe('0x0')
expect(result.result.returnValue).toBe('0x')
expect(result.result.structLogs).toEqual([])

// Keep the existing snapshot for overall structure
expect(result).toMatchInlineSnapshot(/* ... */)

This approach combines the benefits of snapshot testing with clear, specific assertions.

})
7 changes: 5 additions & 2 deletions packages/actions/src/debug/debugTraceTransactionProcedure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,12 @@ export const debugTraceTransactionJsonRpcProcedure = (client) => {
const previousTx = block.transactions.filter(
(_, i) => i < hexToNumber(transactionByHashResponse.result.transactionIndex),
)
const hasStateRoot = vm.stateManager.hasStateRoot(parentBlock.header.stateRoot)

// handle the case where the state root is from a preforked block
const hasStateRoot = await vm.stateManager.hasStateRoot(parentBlock.header.stateRoot)
if (!hasStateRoot && client.forkTransport) {
await forkAndCacheBlock(client, parentBlock)
} else {
} else if (!hasStateRoot) {
return {
jsonrpc: '2.0',
method: request.method,
Expand All @@ -53,6 +55,7 @@ export const debugTraceTransactionJsonRpcProcedure = (client) => {
}
}
const vmClone = await vm.deepCopy()
await vmClone.stateManager.setStateRoot(parentBlock.header.stateRoot)

// execute all transactions before the current one committing to the state
for (const tx of previousTx) {
Expand Down
53 changes: 53 additions & 0 deletions packages/actions/src/debug/debugTraceTransactionProcedure.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { createAddress } from '@tevm/address'
import { SimpleContract } from '@tevm/contract'
import { createTevmNode } from '@tevm/node'
import { describe, expect, it } from 'vitest'
import { callHandler } from '../Call/callHandler.js'
import { deployHandler } from '../Deploy/deployHandler.js'
import { debugTraceTransactionJsonRpcProcedure } from './debugTraceTransactionProcedure.js'

describe('debugTraceTransactionJsonRpcProcedure', () => {
it('should trace a transaction and return the expected result', async () => {
const client = createTevmNode({ miningConfig: { type: 'auto' } })
const procedure = debugTraceTransactionJsonRpcProcedure(client)

const contract = SimpleContract.withAddress(createAddress(420).toString())

await deployHandler(client)(contract.deploy(1n))

const sendTxResult = await callHandler(client)({
createTransaction: true,
...contract.write.set(69n),
})

if (!sendTxResult.txHash) {
throw new Error('Transaction failed')
}

const result = await procedure({
jsonrpc: '2.0',
method: 'debug_traceTransaction',
params: [
{
transactionHash: sendTxResult.txHash,
tracer: 'callTracer',
},
],
id: 1,
})

expect(result).toMatchInlineSnapshot(`
{
"id": 1,
"jsonrpc": "2.0",
"method": "debug_traceTransaction",
"result": {
"failed": false,
"gas": "0x0",
"returnValue": "0x",
"structLogs": [],
},
}
`)
})
})
10 changes: 2 additions & 8 deletions packages/actions/src/eth/ethGetLogsHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { createAddress } from '@tevm/address'
import { ForkError } from '@tevm/errors'
import { createJsonRpcFetcher } from '@tevm/jsonrpc'
import { bytesToHex, hexToBigInt, hexToBytes, numberToHex } from '@tevm/utils'
import { InternalRpcError } from 'viem'
Expand Down Expand Up @@ -70,12 +69,7 @@ export const ethGetLogsHandler = (client) => async (params) => {
],
})
if (error) {
throw new ForkError('Error fetching logs from forked chain', { cause: error })
}
if (!jsonRpcLogs) {
throw new ForkError('Error fetching logs from forked chain no logs returned', {
cause: new Error('Unexpected no logs'),
})
throw error
}
/**
* @typedef {Object} Log
Expand All @@ -94,7 +88,7 @@ export const ethGetLogsHandler = (client) => async (params) => {
/**
* @type {Array<Log> | undefined}
*/
(jsonRpcLogs)
(jsonRpcLogs ?? undefined)

if (typedLogs !== undefined) {
logs.push(
Expand Down
91 changes: 70 additions & 21 deletions packages/actions/src/eth/ethGetLogsHandler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { createAddress } from '@tevm/address'
import { createTevmNode } from '@tevm/node'
import { SimpleContract } from '@tevm/test-utils'
import { SimpleContract, transports } from '@tevm/test-utils'
import {
type Address,
type Hex,
PREFUNDED_ACCOUNTS,
encodeDeployData,
encodeFunctionData,
hexToBytes,
hexToNumber,
keccak256,
stringToHex,
} from '@tevm/utils'
Expand All @@ -33,7 +34,7 @@ describe(ethGetLogsHandler.name, () => {
})
}

it.todo('should return logs for a given block range', async () => {
it('should return logs for a given block range', async () => {
const client = createTevmNode()
const from = createAddress(PREFUNDED_ACCOUNTS[0].address)

Expand All @@ -49,19 +50,26 @@ describe(ethGetLogsHandler.name, () => {
})

const contractAddress = deployResult.createdAddress as Address
expect(deployResult.createdAddresses?.size).toBe(1)
await mineHandler(client)()

// Emit some events
for (let i = 0; i < 3; i++) {
await callHandler(client)({
const res = await callHandler(client)({
to: contractAddress,
from: from.toString(),
data: encodeFunctionData(SimpleContract.write.set(BigInt(i))),
createTransaction: true,
})
expect(res.logs).toHaveLength(1)
await mineHandler(client)()
const { rawData: newValue } = await callHandler(client)({
to: contractAddress,
data: encodeFunctionData(SimpleContract.read.get()),
})
expect(hexToNumber(newValue)).toBe(i)
}

await mineHandler(client)()

const filterParams: FilterParams = {
address: contractAddress,
fromBlock: 0n,
Expand All @@ -75,13 +83,14 @@ describe(ethGetLogsHandler.name, () => {

expect(logs).toHaveLength(3)
expect(logs[0]).toMatchObject({
address: contractAddress,
// this is actually a bug
address: createAddress(contractAddress).toString().toLowerCase(),
Comment on lines +84 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address Normalization Issue

The comment on line 86 indicates there's a bug related to address formatting. In the assertion on line 87, the address is converted to lowercase using .toLowerCase().

Recommendation:

Ethereum addresses are case-insensitive but can be checksummed for error detection. Instead of converting to lowercase, consider using a standard method for address normalization or validation, such as EIP-55 checksumming.

 expect(logs[0]).toMatchObject({
-    address: createAddress(contractAddress).toString().toLowerCase(),
+    address: createAddress(contractAddress).toString(),
     blockNumber: expect.any(BigInt),
     transactionHash: expect.any(String),
 })

If the logged address is in lowercase, ensure that the comparison accounts for the address format used by the logging system.

Committable suggestion was skipped due to low confidence.

blockNumber: expect.any(BigInt),
transactionHash: expect.any(String),
})
})

it.todo('should filter logs by topics', async () => {
it('should filter logs by topics', async () => {
const client = createTevmNode()
const from = createAddress(PREFUNDED_ACCOUNTS[0].address)

Expand All @@ -96,6 +105,9 @@ describe(ethGetLogsHandler.name, () => {

const contractAddress = deployResult.createdAddress as Address

// Mine the deployment transaction
await mineHandler(client)()

// Set values to emit events
await callHandler(client)({
to: contractAddress,
Expand Down Expand Up @@ -129,7 +141,7 @@ describe(ethGetLogsHandler.name, () => {
expect(logs[1]).toBeTruthy()
})

it.todo('should handle pending blocks', async () => {
it('should handle pending blocks', async () => {
const client = createTevmNode()
const from = createAddress(PREFUNDED_ACCOUNTS[0].address)

Expand All @@ -144,6 +156,9 @@ describe(ethGetLogsHandler.name, () => {

const contractAddress = deployResult.createdAddress as Address

// Mine the deployment transaction
await mineHandler(client)()

// Emit an event without mining
await callHandler(client)({
to: contractAddress,
Expand All @@ -164,10 +179,10 @@ describe(ethGetLogsHandler.name, () => {
})

expect(logs).toHaveLength(1)
expect(logs[0]?.blockNumber).toBe('pending')
expect(logs[0]?.blockNumber).toBe(2n)
})

it.todo('should return all logs when no topics are specified', async () => {
it('should return all logs when no topics are specified', async () => {
const client = createTevmNode()
const from = createAddress('0x1234567890123456789012345678901234567890')

Expand All @@ -184,6 +199,9 @@ describe(ethGetLogsHandler.name, () => {

const contractAddress = deployResult.createdAddress as Address

// Mine the deployment transaction
await mineHandler(client)()

// Emit some events
for (let i = 0; i < 3; i++) {
await callHandler(client)({
Expand All @@ -194,23 +212,15 @@ describe(ethGetLogsHandler.name, () => {
})
}

const res = await mineHandler(client)()

const block = await client.getVm().then((vm) => vm.blockchain.getBlock(hexToBytes(res.blockHashes?.[0] as Hex)))

const receiptManager = await client.getReceiptsManager()
block.transactions.forEach(async (tx) => {
const [receipt] = (await receiptManager.getReceiptByTxHash(tx.hash())) ?? []
console.log(receipt?.logs)
})
await mineHandler(client)()

const logs = await ethGetLogsHandler(client)({
filterParams: {},
})

expect(logs).toHaveLength(3)
expect(logs[0]).toMatchObject({
address: contractAddress,
address: contractAddress.toLowerCase(),
blockNumber: expect.any(BigInt),
transactionHash: expect.any(String),
})
Expand All @@ -220,5 +230,44 @@ describe(ethGetLogsHandler.name, () => {
})
})

it.todo('should work fetching logs that were created by tevm after forking')
it('should work for past blocks in forked mode', async () => {
const client = createTevmNode({
fork: {
transport: transports.optimism,
blockTag: 125985200n,
},
})
const logs = await ethGetLogsHandler(client)({
filterParams: {
address: '0xdC6fF44d5d932Cbd77B52E5612Ba0529DC6226F1',
fromBlock: 125985142n,
toBlock: 125985142n,
topics: [
'0x8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925',
'0x0000000000000000000000007f26A7572E8B877654eeDcBc4E573657619FA3CE',
'0x0000000000000000000000007B46fFbC976db2F94C3B3CDD9EbBe4ab50E3d77d',
],
},
})
expect(logs).toHaveLength(1)
expect(logs).toMatchInlineSnapshot(`
[
{
"address": "0xdc6ff44d5d932cbd77b52e5612ba0529dc6226f1",
"blockHash": "0x6c9355482a6937e44fbfbd1c0c9cc95882e47e80c9b48772699c6a49bad1e392",
"blockNumber": 125985142n,
"data": "0x0000000000000000000000000000000000000000000b2f1069a1f95dc7180000",
"logIndex": 23n,
"removed": false,
"topics": [
"0x8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925",
"0x0000000000000000000000007f26a7572e8b877654eedcbc4e573657619fa3ce",
"0x0000000000000000000000007b46ffbc976db2f94c3b3cdd9ebbe4ab50e3d77d",
],
"transactionHash": "0x4f0781ec417fecaf44b248fd0b0485dca9fbe78ad836598b65c12bb13ab9ddd4",
"transactionIndex": 11n,
},
]
`)
Comment on lines +231 to +269
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance Test Robustness for Forked Mode

The test uses hardcoded block numbers and addresses specific to the forked network.

Suggestion:

To improve the test's robustness and maintainability:

  • Parametrize Block Numbers and Addresses: Extract the block numbers and addresses into constants or configuration files. This makes it easier to update them if the underlying data changes.
  • Check for Empty Logs: Add assertions to handle cases where the logs array might be empty due to changes in the forked chain state.
const FORK_BLOCK_NUMBER = 125985200n
const TEST_BLOCK_NUMBER = 125985142n
const TEST_ADDRESS = '0xdC6fF44d5d932Cbd77B52E5612Ba0529DC6226F1'

const client = createTevmNode({
    fork: {
        transport: transports.optimism,
        blockTag: FORK_BLOCK_NUMBER,
    },
})

const logs = await ethGetLogsHandler(client)({
    filterParams: {
        address: TEST_ADDRESS,
        fromBlock: TEST_BLOCK_NUMBER,
        toBlock: TEST_BLOCK_NUMBER,
        topics: [...],
    },
})

expect(logs).not.toHaveLength(0)
expect(logs[0]?.blockNumber).toBe(TEST_BLOCK_NUMBER)

This approach enhances clarity and eases future updates.

})
})
Loading
Loading