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.

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

✅ Test: Add a bunch of viem api tests #1109

Merged

Conversation

roninjin10
Copy link
Collaborator

@roninjin10 roninjin10 commented May 20, 2024

Description

Concise description of proposed changes

Testing

Explain the quality checks that have been done on the code changes

Additional Information

Your ENS/address:

Summary by CodeRabbit

  • Bug Fixes

    • Resolved issues with impersonated transactions not being copied correctly into new blocks.
    • Fixed incorrect reading of some storage slots.
    • Corrected ethGetCode to update properly after blockchain refactor.
    • Addressed decoding issues by setting blobGasUsed correctly.
    • Fixed last block's state root mutation during new block mining.
    • Made blobGasPrice and blobGasUsed fields optional in transaction receipts.
    • Improved error messages for storage key length validation.
  • Chores

    • Removed console.log statements from various methods to clean up console output.

Copy link

vercel bot commented May 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tevm-monorepo-tevm ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 9:01am

Copy link

changeset-bot bot commented May 20, 2024

🦋 Changeset detected

Latest commit: 8e6e9a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@tevm/actions-types Patch
@tevm/contract Patch
@tevm/test-utils Patch
@tevm/block Patch
@tevm/actions Patch
@tevm/procedures Patch
@tevm/receipt-manager Patch
@tevm/decorators Patch
@tevm/server Patch
tevm Patch
@tevm/experimental-solc Patch
@tevm/ethers Patch
@tevm/opstack Patch
@tevm/http-client Patch
@tevm/memory-client Patch
@tevm/precompiles Patch
@tevm/predeploys Patch
@tevm/base-client Patch
@tevm/state Patch
@tevm/blockchain Patch
@tevm/txpool Patch
@tevm/vm Patch
@tevm/bundler Patch
@tevm/esbuild-plugin Patch
@tevm/rollup-plugin Patch
@tevm/rspack-plugin Patch
@tevm/vite-plugin Patch
@tevm/webpack-plugin Patch
@tevm/evm Patch
@tevm/sync-storage-persister Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

coderabbitai bot commented May 20, 2024

Warning

Rate Limit Exceeded

@roninjin10 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 48 minutes and 5 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 8ce819d and 8e6e9a8.

Walkthrough

This update primarily addresses bug fixes across several packages in the @tevm ecosystem. The changes include fixing transaction handling issues, removing unnecessary console logs, correcting storage slot reads, and updating type declarations. Additionally, there are improvements in block and transaction management, error handling, and state root updates. These modifications aim to enhance stability and accuracy in transaction processing and blockchain state management.

Changes

File Path Change Summary
.changeset/flat-badgers-marry.md, .changeset/eighty-cameras-lay.md Fixed bugs related to transaction copying and bytecode setting.
.changeset/popular-eggs-train.md, packages/receipt-manager/src/RecieptManager.ts Removed console.log statements.
.changeset/tame-foxes-play.md, packages/actions/src/eth/getStorageAtHandler.js Fixed storage slot read issues.
.changeset/gentle-cars-fail.md, packages/actions/src/eth/getCodeHandler.js Fixed ethGetCode bug and updated block tag handling.
.changeset/perfect-suns-pull.md, packages/procedures/src/eth/ethGetTransactionReceiptProcedure.js Fixed blobGasUsed bug and updated transaction receipt handling.
.changeset/spicy-rice-tickle.md, packages/actions/src/tevm/mineHandler.js Fixed state root mutation during block mining.
.changeset/cuddly-phones-impress.md, docs/src/content/docs/reference/@tevm/actions-types/type-aliases/TransactionReceiptResult.md Made blobGasPrice and blobGasUsed fields optional in transaction receipt type.
packages/block/src/block.ts, packages/vm/src/actions/buildBlock.ts, packages/vm/src/actions/runBlock.ts Updated transaction handling and state root setting in block processing.
packages/memory-client/src/test/viemPublicActions.spec.ts Adjusted test cases and imports for various public actions.
packages/state/src/actions/getContractStorage.js, packages/state/src/actions/putContractStorage.js Improved error messages for storage key length validation.

In code we trust, through bugs we wade,
Fixing issues, improvements made.
Transactions clear, storage bright,
Blocks and bytes now set just right.
Logs removed, errors caught,
A stable system, finely wrought.
🐰✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @roninjin10 and the rest of your teammates on Graphite Graphite

@roninjin10 roninjin10 marked this pull request as ready for review May 20, 2024 18:02
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Out of diff range and nitpick comments (1)
packages/memory-client/src/test/viemPublicActions.spec.ts (1)

74-76: Use toMatchSnapshot judiciously to ensure meaningful test comparisons.

Consider using explicit assertions for known values instead of snapshots where possible to make the tests more understandable and maintainable.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 3c02464 and d8f0b74.
Files selected for processing (1)
  • packages/memory-client/src/test/viemPublicActions.spec.ts (4 hunks)
Additional comments not posted (6)
packages/memory-client/src/test/viemPublicActions.spec.ts (6)

3-5: Ensure new imports are utilized appropriately in the test cases.


9-31: Define eventAbi with a complete structure.


35-35: Verify the type and usage of deployTxHash.

Verification successful

The type Hex is defined as a template literal type 0x${string}, which is appropriate for representing hexadecimal strings. The usage of deployTxHash in the test file packages/memory-client/src/test/viemPublicActions.spec.ts aligns with this type definition.

  • deployTxHash is assigned a value from deployResult.txHash.
  • It is used as a parameter in various method calls, such as mc.getTransaction, mc.getTransactionConfirmations, and mc.getTransactionReceipt.

These usages are consistent with the expected type Hex.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the type and usage of `deployTxHash` across the codebase.

# Test: Search for the variable usage. Expect: Correct type and usage.
rg --type typescript $'deployTxHash'

Length of output: 70



Script:

#!/bin/bash
# Description: Verify the type and usage of `deployTxHash` across the codebase.

# Test: Search for the variable usage. Expect: Correct type and usage.
rg --type ts 'deployTxHash'

Length of output: 622



Script:

#!/bin/bash
# Description: Verify the definition and usage of the type `Hex`.

# Test: Search for the definition of `Hex`. Expect: Correct type definition.
rg --type ts 'type Hex' 

Length of output: 7821


255-258: Verify the behavior of simulateContract with realistic data.

Verification successful

The search results indicate that simulateContract is tested in multiple files, including viemPublicActions.spec.ts, createClient.test.ts, createTestClient.test.ts, public.test.ts, writeContract.test.ts, and simulateContract.test.ts. These tests should provide sufficient coverage to verify the behavior of simulateContract with realistic data.

  • packages/memory-client/src/test/viemPublicActions.spec.ts
  • experimental/viem-effect/src-viem/clients/createClient.test.ts
  • experimental/viem-effect/src-viem/clients/createTestClient.test.ts
  • experimental/viem-effect/src-viem/clients/decorators/public.test.ts
  • experimental/viem-effect/src-viem/actions/public/simulateContract.test.ts
  • experimental/viem-effect/src-viem/actions/wallet/writeContract.test.ts

These files contain various test cases that utilize simulateContract, ensuring its functionality is tested under different scenarios.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the behavior of `simulateContract` by using realistic data.

# Test: Search for additional tests or data that could be used to verify this function.
rg --type typescript $'simulateContract'

Length of output: 74



Script:

#!/bin/bash
# Description: Verify the behavior of `simulateContract` by using realistic data.

# Test: Search for additional tests or data that could be used to verify this function.
rg --type ts 'simulateContract'

Length of output: 15907



Script:

#!/bin/bash
# Description: Extract test cases for `simulateContract` to verify the use of realistic data.

# Extract test cases from relevant test files
rg --type ts --files-with-matches 'simulateContract' | grep -E 'test|spec' | xargs rg 'simulateContract'

Length of output: 10954


122-129: Confirm the accuracy of the gas estimate.


199-203: Validate the getGasPrice test for realistic scenarios.

Comment on lines +206 to +210
it.todo('should work', async () => {
const filter = await mc.createEventFilter(eventAbi)
const logs = await mc.getLogs(filter)
expect(logs).toHaveLength(0)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure getLogs test is complete and covers all necessary assertions.

The test marked with todo should be completed to ensure it covers all necessary assertions for the getLogs function.

Comment on lines +157 to +158
// TODO
expect(await mc.getBlockTransactionCount({ blockTag: 'latest' })).toBe(10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Complete the implementation of getBlockTransactionCount.

It appears this test is incomplete. Would you like assistance in implementing this test?

Comment on lines 105 to 111
it('works with args: event', async () => {
const filter = await mc.createEventFilter(eventAbi)
expect(filter.args).toBeUndefined()
// @ts-expect-errory TODO this is a viem test copy pasted why is type not working?
expect(filter.abi).toEqual([eventAbi])
expect(filter.eventName).toEqual('Transfer')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Address TypeScript error and ensure the test reflects the intended functionality.

The TypeScript error needs resolution, and the test should accurately reflect the expected behavior of the createEventFilter function when using the event argument.

Comment on lines 78 to 87
createContractEventFilter: async () => {
it('works', async () => {
const filter = await mc.createContractEventFilter({
abi: c.simpleContract.abi,
})
expect(filter.id).toBeDefined()
expect(filter.type).toBe('event')
expect(filter.args).toBeUndefined()
expect(filter.eventName).toBeUndefined()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the expected behavior and properties of createContractEventFilter.

It seems the test lacks assertions for some expected properties. Consider adding checks for args and eventName if they are supposed to be set.

Comment on lines +136 to +138
const TODO = 0n
expect(await mc.getBlobBaseFee()).toBe(TODO)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace placeholder in getBlobBaseFee test with actual implementation.

- const TODO = 0n
+ const actualFee = await someMethodToFetchBlobBaseFee(); // Replace with actual method call
- expect(await mc.getBlobBaseFee()).toBe(TODO)
+ expect(await mc.getBlobBaseFee()).toBe(actualFee)

Comment on lines 99 to 103
it('works with args: address', async () => {
await mc.createEventFilter({
address: `0x${'69'.repeat(20)}`,
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Add assertions to verify the behavior of createEventFilter with address.

+ expect(filter).toBeDefined();

Comment on lines +53 to +56
if (!deployResult.txHash) {
throw new Error('txHash not found')
}
deployTxHash = deployResult.txHash
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle potential exceptions from missing txHash more gracefully.

- if (!deployResult.txHash) {
-   throw new Error('txHash not found')
- }
+ if (!deployResult.txHash) {
+   console.error('Transaction hash not found. Please check the deployment process.');
+   return;
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if (!deployResult.txHash) {
throw new Error('txHash not found')
}
deployTxHash = deployResult.txHash
if (!deployResult.txHash) {
console.error('Transaction hash not found. Please check the deployment process.');
return;
}
deployTxHash = deployResult.txHash

Comment on lines 163 to 165
// this will fail because bytecode is wrong
expect(await mc.getBytecode({ address: c.simpleContract.address })).toBe(simpleContract.bytecode)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct the expected value for bytecode in getBytecode.

- expect(await mc.getBytecode({ address: c.simpleContract.address })).toBe(simpleContract.bytecode)
+ expect(await mc.getBytecode({ address: c.simpleContract.address })).toEqual(simpleContract.bytecode)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// this will fail because bytecode is wrong
expect(await mc.getBytecode({ address: c.simpleContract.address })).toBe(simpleContract.bytecode)
})
// this will fail because bytecode is wrong
expect(await mc.getBytecode({ address: c.simpleContract.address })).toEqual(simpleContract.bytecode)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between d8f0b74 and 037e428.
Files ignored due to path filters (1)
  • packages/memory-client/src/test/__snapshots__/viemPublicActions.spec.ts.snap is excluded by !**/*.snap
Files selected for processing (11)
  • .changeset/flat-badgers-marry.md (1 hunks)
  • .changeset/popular-eggs-train.md (1 hunks)
  • .changeset/tame-foxes-play.md (1 hunks)
  • packages/actions/src/eth/ethGetTransactionReceipt.js (1 hunks)
  • packages/actions/src/eth/getStorageAtHandler.js (1 hunks)
  • packages/block/src/block.ts (1 hunks)
  • packages/memory-client/src/test/viemPublicActions.spec.ts (4 hunks)
  • packages/receipt-manager/src/RecieptManager.ts (2 hunks)
  • packages/state/src/actions/getContractStorage.js (1 hunks)
  • packages/state/src/actions/putContractStorage.js (2 hunks)
  • packages/vm/src/actions/buildBlock.ts (4 hunks)
Files skipped from review due to trivial changes (3)
  • .changeset/popular-eggs-train.md
  • packages/actions/src/eth/ethGetTransactionReceipt.js
  • packages/receipt-manager/src/RecieptManager.ts
Additional comments not posted (9)
.changeset/tame-foxes-play.md (1)

1-5: The changeset description is clear and concise.

.changeset/flat-badgers-marry.md (1)

1-5: The changeset description is clear and concise.

packages/state/src/actions/putContractStorage.js (1)

22-22: Enhanced error message provides better clarity and debugging support.

packages/actions/src/eth/getStorageAtHandler.js (1)

14-42: The updates to handle different scenarios based on the tag value and the addition of specific error handling improve the function's robustness and clarity.

packages/state/src/actions/getContractStorage.js (1)

19-19: Enhanced error message provides better clarity and debugging support.

packages/memory-client/src/test/viemPublicActions.spec.ts (1)

3-5: New imports and the eventAbi variable are correctly added and used in the tests.

Also applies to: 9-31

packages/vm/src/actions/buildBlock.ts (3)

33-33: Ensure the new import of ImpersonatedTx is utilized correctly in the context of this file.


61-61: The modification to allow ImpersonatedTx in the transactions array aligns with the broader system's need to handle different transaction types dynamically.


203-203: Review the handling of ImpersonatedTx in addTransaction method to ensure it integrates well with the existing transaction processing logic, especially regarding gas calculations and state changes.

Comment on lines +74 to +76
it.todo('works', async () => {
expect(await mc.createBlockFilter()).toMatchSnapshot()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Several tests are marked with todo. Completing these will ensure comprehensive coverage and functionality verification.

Would you like assistance in completing these tests or perhaps setting up a task to track their completion?

Also applies to: 78-87, 90-111, 114-114, 134-136, 139-142, 160-162, 167-169, 204-207, 210-214, 223-229

@@ -125,7 +126,7 @@ export class BlockBuilder {
* Calculates and returns the transactionsTrie for the block.
*/
public async transactionsTrie() {
return Block.genTransactionsTrieRoot(this.transactions, new Trie({ common: this.vm.common }))
return Block.genTransactionsTrieRoot(/** @type {Array<TypedTransaction>}*/(this.transactions), new Trie({ common: this.vm.common }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The genTransactionsTrieRoot method in packages/block/src/block.ts does not include any specific handling for ImpersonatedTx types. It simply iterates over the transactions, serializes them, and inserts them into a trie.

  • packages/vm/src/actions/buildBlock.ts: The transactionsTrie method casts this.transactions to Array<TypedTransaction> without additional handling for ImpersonatedTx.
  • packages/block/src/block.ts: The genTransactionsTrieRoot method processes transactions generically without distinguishing ImpersonatedTx.

This indicates that the type casting to Array<TypedTransaction> might omit necessary handling for ImpersonatedTx types.

Analysis chain

Confirm that the type casting to Array<TypedTransaction> does not omit necessary handling for ImpersonatedTx types, given their inclusion in the transactions array.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that `ImpersonatedTx` transactions are handled correctly in `transactionsTrie`.

# Test: Search for handling of `ImpersonatedTx` in the method `transactionsTrie`.
ast-grep --lang javascript --pattern $'class BlockBuilder {
  $$$
  transactionsTrie($_) {
    $$$
  }
  $$$  
}'

Length of output: 114



Script:

#!/bin/bash
# Description: Verify the existence and implementation of the `transactionsTrie` method in the codebase.

# Search for the `transactionsTrie` method in the entire codebase.
ast-grep --lang javascript --pattern $'transactionsTrie($_) {
  $$$
}'

Length of output: 524



Script:

#!/bin/bash
# Description: Verify the existence and implementation of the `transactionsTrie` method in the codebase.

# Search for the `transactionsTrie` method in the entire codebase with corrected pattern syntax.
ast-grep --lang javascript --pattern $'function transactionsTrie($_) {
  $$$
}'

Length of output: 80



Script:

#!/bin/bash
# Description: Verify the existence and implementation of the `transactionsTrie` method in the codebase.

# Search for the `transactionsTrie` method name in the entire codebase.
rg 'transactionsTrie'

Length of output: 3421



Script:

#!/bin/bash
# Description: Extract the implementation of the `transactionsTrie` method from `packages/vm/src/actions/buildBlock.ts`.

# Extract the implementation of the `transactionsTrie` method.
ast-grep --lang typescript --pattern $'public async transactionsTrie() {
  $$$
}' packages/vm/src/actions/buildBlock.ts

Length of output: 584



Script:

#!/bin/bash
# Description: Extract the implementation of the `transactionsTrie` method from `packages/vm/src/actions/buildBlock.ts`.

# Extract the implementation of the `transactionsTrie` method.
ast-grep --lang typescript --pattern $'async transactionsTrie() {
  $$$
}' packages/vm/src/actions/buildBlock.ts

Length of output: 570



Script:

#!/bin/bash
# Description: Extract the implementation of the `transactionsTrie` method from `packages/vm/src/actions/buildBlock.ts`.

# Extract the implementation of the `transactionsTrie` method.
rg 'async transactionsTrie' -A 20 packages/vm/src/actions/buildBlock.ts

Length of output: 682



Script:

#!/bin/bash
# Description: Extract the implementation of the `genTransactionsTrieRoot` method from the `Block` class.

# Search for the `genTransactionsTrieRoot` method in the entire codebase.
ast-grep --lang typescript --pattern $'static async genTransactionsTrieRoot($_) {
  $$$
}' 

Length of output: 564



Script:

#!/bin/bash
# Description: Extract the implementation of the `genTransactionsTrieRoot` method from the `Block` class.

# Search for the `genTransactionsTrieRoot` method in the entire codebase.
rg 'static async genTransactionsTrieRoot' -A 20

Length of output: 1533

Comment on lines 124 to 126
// We should make a new tx using header common in case of setHardfork being activated
// We aren't doing that atm because it causes a bug with impersonated transactions
transactions.push(txData)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments indicate a known issue with handling impersonated transactions when setting hardforks. This needs attention to ensure that transactions are processed correctly across different network conditions and configurations.

- // We aren't doing that atm because it causes a bug with impersonated transactions
+ // TODO: Resolve the bug with impersonated transactions when setting hardforks.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// We should make a new tx using header common in case of setHardfork being activated
// We aren't doing that atm because it causes a bug with impersonated transactions
transactions.push(txData)
// We should make a new tx using header common in case of setHardfork being activated
// TODO: Resolve the bug with impersonated transactions when setting hardforks.
transactions.push(txData)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Outside diff range and nitpick comments (1)
packages/actions/src/eth/getCodeHandler.js (1)

22-22: Consider handling specific error types more robustly in future updates.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 037e428 and be0ada8.
Files ignored due to path filters (1)
  • packages/memory-client/src/test/__snapshots__/viemPublicActions.spec.ts.snap is excluded by !**/*.snap
Files selected for processing (16)
  • .changeset/eighty-cameras-lay.md (1 hunks)
  • .changeset/gentle-cars-fail.md (1 hunks)
  • .changeset/perfect-suns-pull.md (1 hunks)
  • .changeset/spicy-rice-tickle.md (1 hunks)
  • packages/actions/src/eth/ethGetTransactionReceipt.js (1 hunks)
  • packages/actions/src/eth/getCodeHandler.js (1 hunks)
  • packages/actions/src/eth/getStorageAtHandler.js (1 hunks)
  • packages/actions/src/tevm/mineHandler.js (1 hunks)
  • packages/contract/src/contract-lib/SimpleContract.ts (1 hunks)
  • packages/memory-client/src/test/viemPublicActions.spec.ts (4 hunks)
  • packages/procedures/src/eth/ethGetTransactionReceiptProcedure.js (2 hunks)
  • packages/state/src/actions/getContractStorage.js (1 hunks)
  • packages/state/src/actions/putContractStorage.js (1 hunks)
  • packages/vm/src/actions/buildBlock.ts (4 hunks)
  • packages/vm/src/actions/runBlock.ts (1 hunks)
  • test/test-utils/src/simpleContract.ts (1 hunks)
Files skipped from review due to trivial changes (2)
  • packages/procedures/src/eth/ethGetTransactionReceiptProcedure.js
  • test/test-utils/src/simpleContract.ts
Files skipped from review as they are similar to previous changes (6)
  • packages/actions/src/eth/ethGetTransactionReceipt.js
  • packages/actions/src/eth/getStorageAtHandler.js
  • packages/memory-client/src/test/viemPublicActions.spec.ts
  • packages/state/src/actions/getContractStorage.js
  • packages/state/src/actions/putContractStorage.js
  • packages/vm/src/actions/buildBlock.ts
Additional comments not posted (10)
.changeset/gentle-cars-fail.md (1)

5-5: The changeset description is clear and accurately describes the bug fix for ethGetCode.

.changeset/spicy-rice-tickle.md (1)

5-5: The changeset description clearly explains the bug fix related to the state root mutation during block mining.

.changeset/perfect-suns-pull.md (1)

5-5: The changeset description clearly explains the bug fix related to blobGasUsed being incorrectly set to '0x'.

.changeset/eighty-cameras-lay.md (1)

6-6: The changeset description clearly explains the bug fix related to the bytecode and deployedBytecode fields in simpleContract.

packages/actions/src/eth/getCodeHandler.js (1)

17-22: The updates to handle cases where forkUrl is not provided are clear and enhance the robustness of the function.

packages/actions/src/tevm/mineHandler.js (4)

15-15: Ensure proper defaulting of throwOnFail parameter.

The use of throwOnFail = options.throwOnFail ?? true ensures that the function defaults to throwing errors unless explicitly overridden, which is a safe default behavior.


16-16: Good use of structured logging.

Logging both the throwOnFail flag and additional parameters provides clear diagnostics which can be crucial for debugging.


17-19: Validate input parameters effectively.

Using validateMineParams to check the validity of input parameters and handling potential errors by possibly throwing them is a robust approach to ensure the function's integrity.


33-106: Review the transaction processing and block building logic.

The loop from lines 33 to 106 handles the core mining operations, including transaction processing and block building. It's crucial to ensure that this logic is consistent with other parts of the system handling similar operations.

Verification successful

The search for blockBuilder.addTransaction only returned a result from the mineHandler.js file itself, indicating that this might be the only place where this specific logic is implemented. Let's review the transaction processing and block building logic in the provided code snippet to ensure it is correct and consistent.

Analysis of the Code Snippet

  1. Transaction Processing Loop:

    • The loop processes transactions by price and nonce, adding them to the block until the block is full.
    • It logs each transaction added and handles exceptions, particularly out-of-gas errors.
  2. Block Building:

    • The block is built with a new timestamp and incremented block number.
    • The block is committed to the blockchain, and receipts are saved.
  3. State Management:

    • The state is checkpointed and committed.
    • The state root is saved to the original VM's state manager.

Potential Issues and Considerations

  • Exception Handling: The code handles out-of-gas exceptions but should ensure other potential exceptions are also managed.
  • State Root Verification: The code verifies the state root exists before saving it, which is good practice.
  • Concurrency: Ensure that the asynchronous operations (e.g., await vm.stateManager.commit) are correctly handled to avoid race conditions.

Conclusion

The transaction processing and block building logic in mineHandler.js appears to be well-structured and follows best practices. However, since this is the only instance found, it is crucial to ensure that any changes to this logic are thoroughly tested.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the transaction processing logic and block building in `mineHandler`.

# Test: Search for similar patterns in other mining handlers. Expect: Consistent handling.
rg --type javascript "blockBuilder.addTransaction"

Length of output: 85



Script:

#!/bin/bash
# Description: Verify the transaction processing logic and block building in `mineHandler`.

# Test: Search for similar patterns in other mining handlers. Expect: Consistent handling.
rg --type js "blockBuilder.addTransaction"

Length of output: 164

packages/vm/src/actions/runBlock.ts (1)

94-94: Ensure state root is set correctly.

Setting the state root explicitly after processing a block is a critical step for maintaining the integrity of the blockchain state.

packages/actions/src/tevm/mineHandler.js Outdated Show resolved Hide resolved
.changeset/eighty-cameras-lay.md Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@ export const SimpleContract = createScript({
'function set(uint256 x)',
] as const,
bytecode:
'0x608060405234801561000f575f80fd5b506040516101ef3803806101ef83398181016040528101906100319190610074565b805f819055505061009f565b5f80fd5b5f819050919050565b61005381610041565b811461005d575f80fd5b50565b5f8151905061006e8161004a565b92915050565b5f602082840312156100895761008861003d565b5b5f61009684828501610060565b91505092915050565b610143806100ac5f395ff3fe608060405234801561000f575f80fd5b5060043610610034575f3560e01c806360fe47b1146100385780636d4ce63c14610054575b5f80fd5b610052600480360381019061004d91906100ba565b610072565b005b61005c61007b565b60405161006991906100f4565b60405180910390f35b805f8190555050565b5f8054905090565b5f80fd5b5f819050919050565b61009981610087565b81146100a3575f80fd5b50565b5f813590506100b481610090565b92915050565b5f602082840312156100cf576100ce610083565b5b5f6100dc848285016100a6565b91505092915050565b6100ee81610087565b82525050565b5f6020820190506101075f8301846100e5565b9291505056fea264697066735822122019e943356c89506511b952171a3b4724d3152e5f4029bbb0ecc836d1365fcce464736f6c63430008160033',
'0x40516101ef3803806101ef83398181016040528101906100319190610074565b805f819055505061009f565b5f80fd5b5f819050919050565b61005381610041565b811461005d575f80fd5b50565b5f8151905061006e8161004a565b92915050565b5f602082840312156100895761008861003d565b5b5f61009684828501610060565b91505092915050565b610143806100ac5f395ff3fe608060405234801561000f575f80fd5b5060',
Copy link
Contributor

@coderabbitai coderabbitai bot May 21, 2024

Choose a reason for hiding this comment

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

Ensure that the deployedBytecode is updated as indicated by the TODO comment to avoid deployment issues.

Would you like me to help update the deployedBytecode or should we track this as an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should remove the todo comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@roninjin10, removing the TODO comment might clean up the code, but it could also obscure the fact that the deployedBytecode might still need to be updated. This could potentially lead to deployment issues in the future if not addressed.

Would you like to proceed with updating the deployedBytecode first, or should we open a GitHub issue to track this necessary update before removing the TODO comment? This way, we can ensure that the issue is not forgotten and is addressed appropriately in the future.

William Cory and others added 6 commits May 21, 2024 00:08
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between be0ada8 and 8ce819d.
Files ignored due to path filters (1)
  • packages/memory-client/src/test/__snapshots__/viemPublicActions.spec.ts.snap is excluded by !**/*.snap
Files selected for processing (15)
  • .changeset/cuddly-phones-impress.md (1 hunks)
  • .changeset/eighty-cameras-lay.md (1 hunks)
  • docs/src/content/docs/reference/@tevm/actions-types/type-aliases/TransactionReceiptResult.md (1 hunks)
  • packages/actions-types/docs/type-aliases/TransactionReceiptResult.md (1 hunks)
  • packages/actions-types/src/common/TransactionReceiptResult.ts (1 hunks)
  • packages/actions/src/tevm/mineHandler.js (1 hunks)
  • packages/actions/src/tevm/mineHandler.spec.ts (1 hunks)
  • packages/block/docs/classes/Block.md (24 hunks)
  • packages/block/src/block.ts (2 hunks)
  • packages/contract/src/contract-lib/SimpleContract.ts (1 hunks)
  • packages/memory-client/src/test/viemPublicActions.spec.ts (4 hunks)
  • packages/receipt-manager/docs/classes/ReceiptsManager.md (12 hunks)
  • packages/vm/src/actions/buildBlock.ts (6 hunks)
  • test/test-utils/src/simpleContract.ts (1 hunks)
  • tevm/docs/actions-types/type-aliases/TransactionReceiptResult.md (1 hunks)
Files skipped from review due to trivial changes (3)
  • packages/actions-types/docs/type-aliases/TransactionReceiptResult.md
  • packages/actions/src/tevm/mineHandler.spec.ts
  • packages/receipt-manager/docs/classes/ReceiptsManager.md
Files skipped from review as they are similar to previous changes (6)
  • .changeset/eighty-cameras-lay.md
  • packages/block/src/block.ts
  • packages/contract/src/contract-lib/SimpleContract.ts
  • packages/memory-client/src/test/viemPublicActions.spec.ts
  • packages/vm/src/actions/buildBlock.ts
  • test/test-utils/src/simpleContract.ts
Additional comments not posted (6)
.changeset/cuddly-phones-impress.md (1)

1-5: The changeset description accurately reflects the modifications made to the @tevm/actions-types package regarding the blob parameters. Good job on maintaining clear and concise documentation for version changes.

packages/actions-types/src/common/TransactionReceiptResult.ts (1)

20-21: The changes to make blobGasUsed and blobGasPrice optional are correctly implemented. This adjustment enhances flexibility in handling transaction receipts where these values might not be available.

tevm/docs/actions-types/type-aliases/TransactionReceiptResult.md (1)

15-21: The documentation updates correctly reflect the changes to the TransactionReceiptResult type, marking blobGasUsed and blobGasPrice as optional. It's crucial to keep the documentation aligned with the codebase, and this update achieves that.

docs/src/content/docs/reference/@tevm/actions-types/type-aliases/TransactionReceiptResult.md (1)

14-20: The updates to the documentation here are consistent with the changes to the TransactionReceiptResult type and other documentation sources, ensuring comprehensive and synchronized documentation across the board.

packages/actions/src/tevm/mineHandler.js (1)

14-111: The enhancements to the mineHandler function introduce additional logic for processing transactions and building blocks. It's important to ensure that these changes are thoroughly tested, especially since they affect the core functionality of block creation and transaction processing.

packages/block/docs/classes/Block.md (1)

Line range hint 42-629: The updates to the line numbers in the Block class documentation are correctly applied, ensuring that the documentation accurately reflects the source code. This is crucial for developers who rely on accurate documentation for reference.

@roninjin10 roninjin10 force-pushed the 05-20-_white_check_mark_test_add_a_bunch_of_viem_api_tests branch from fcd4d43 to 8e6e9a8 Compare May 21, 2024 08:31
@roninjin10 roninjin10 merged commit 9eeba47 into main May 21, 2024
15 of 16 checks passed
@roninjin10 roninjin10 deleted the 05-20-_white_check_mark_test_add_a_bunch_of_viem_api_tests branch May 21, 2024 08:36
roninjin10 pushed a commit that referenced this pull request May 21, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`main` is currently in **pre mode** so this branch has prereleases
rather than normal releases. If you want to exit prereleases, run
`changeset pre exit` on `main`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

-   Updated dependencies \[]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed bug with
ethGetCode not getting updated post blockchain refactor

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fix: Bug with
last blocks state root accidentally being mutated when mining new blocks

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed bug in
getStorageAtHandler preventing some storage slots to not be read
correctly.

- Updated dependencies
\[[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed bug with
blob params in receipt return type being required rather than optional

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed bug in
@tevm/block where impersonated tx were not being properly copied into
newly created blocks

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed copy-pasta
bug with copying deployedBytecode into bytecode for simpleContract. The
correct bytecode is now set.

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

-   Updated dependencies \[]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed bug with
blobGasUsed being '0x' rather than undefined. This causes issues with
viem decoding.

- Updated dependencies
\[[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Removed
console.log

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

-   Updated dependencies \[]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

## @tevm/[email protected]

### Patch Changes

- [#1109](#1109)
[`9eeba47`](9eeba47)
Thanks [@roninjin10](https://github.com/roninjin10)! - Fixed copy-pasta
bug with copying deployedBytecode into bytecode for simpleContract. The
correct bytecode is now set.

- Updated dependencies
\[[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]

## [email protected]

### Patch Changes

- Updated dependencies
\[[`9eeba47`](9eeba47),
[`9eeba47`](9eeba47)]:
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]
    -   @tevm/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant