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

feat: getLogs cheatcode #5297

Merged
merged 24 commits into from
Aug 21, 2023
Merged

feat: getLogs cheatcode #5297

merged 24 commits into from
Aug 21, 2023

Conversation

puma314
Copy link
Contributor

@puma314 puma314 commented Jul 5, 2023

Motivation

Right now there is a getRecordedLogs() cheatcode that allows users to call vm.recordLogs() and retrieve subsequent emitted logs for post-processing or other use-cases.

However, there is no way for users to retrieve logs that are emitted outside of the recordLogs() context.

A lot developers use one-off Typescript and Go scripts to retrieve logs and then conditionally call other Solidity functions with them. This requires significant duplication of existing Solidity logic in other languages. If these scripts could be replaced with Forge scripts that are able to interact with existing Solidity logic, it would simplify off-chain workflows significantly. The ability to retrieve logs in a similar manner to the getLogs RPC within a Forge script is the main blocker to using Forge scripts for these sorts of tasks.

Solution

Create a new cheatcode getLogs(uint256 fromBlock, uint256 toBlock, address filterAddress, bytes32[] memory topics) with the same arguments as the eth_getLogs RPC.

If there is an active fork, use the fork_url to query the getLogs RPC and return the result as a vm.Logs[] object. If there is no active fork, then throw an error.

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

thanks for this! it's going in the right direction. let's keep resolving the TODOs left, and:

  • making the test data a fixture
  • removing the unneeded comments once todos are solved
  • extract the cheatcode itself into a function.

also, left a nit re: formatting.

evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
}

/* The response from the above eth_call
// TODO put into a fixture
Copy link
Member

Choose a reason for hiding this comment

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

sweet yes! i'd also include some comments on how to regen this fixture, if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included in README.md under testdata/fixtures/Rpc folder.

return Some(Ok(empty));
*/

let logs = RuntimeOrHandle::new().block_on(provider.get_logs(&filter)).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

cc @mattsse — ig there's no way to avoid spawning a blocking handle here right?

evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
@mds1
Copy link
Collaborator

mds1 commented Jul 5, 2023

This PR basically adds a new vm.getLogs cheat which is just a way to call the eth_getLogs RPC method natively inside a test/script, without vm.ffi, which is great and definitely valuable

The ability to call RPC methods from tests/scripts has been a request for a long time, often to allow configuring anvil into a specified state from a forge script. So before merging a one-off cheat for this method I think it's worth framing this more broadly around "how to call RPC methods from script/tests". I see three options:

  1. Native cheats for each RPC method. Downside here is that this ends up adding a lot of cheats, and you need bespoke cheats for each chain.
    • With this approach, we may want to rename this to vm.eth_getLogs to match the RPC method—we should think about the naming convention otherwise we pollute the cheatcode namespace with lots of RPC cheats that aren't easily identifiable as "aliases for RPC calls", since that's not immediately clear from vm.getLogs
  2. A single vm.rpc(string memory name, bytes memory args) returns (bytes memory returnData) cheat that takes the method name and ABI-encoded args, which are decoded and sent to the RPC. The return data is ABI-encoded and can be decoded in solidity. This scales nicely for arbitrary RPC calls on any chain
  3. A hybrid approach where common/standardized methods have dedicated cheats and we have the vm.rpc escape hatch for the rest

I lean towards 2 or 3, and also lean towards just matching the full RPC method name for clarity. Interested to hear other thoughts on this

@puma314
Copy link
Contributor Author

puma314 commented Jul 10, 2023

I lean towards 2 or 3, and also lean towards just matching the full RPC method name for clarity. Interested to hear other thoughts on this

I also strongly support matching the full RPC method name, that's a great suggestion. We can also name the return type as EthGetLogsResponse[] (or I'm open to other naming suggestions/conventions here since that name is a bit verbose) because the standard Foundry vm.Logs only has a subset of values returned by the RPC and I've found all the returned values to be useful at some point or another.

Between approaches 2 and 3, I lean towards 3 because I think there are strong benefits for type-safety for the most common RPC calls, but the vm.rpc escape hatch seems nice for all the rest.

@mds1
Copy link
Collaborator

mds1 commented Jul 10, 2023

I also strongly support matching the full RPC method name, that's a great suggestion. We can also name the return type as EthGetLogsResponse[] (or I'm open to other naming suggestions/conventions here since that name is a bit verbose) because the standard Foundry vm.Logs only has a subset of values returned by the RPC and I've found all the returned values to be useful at some point or another.

Between approaches 2 and 3, I lean towards 3 because I think there are strong benefits for type-safety for the most common RPC calls, but the vm.rpc escape hatch seems nice for all the rest.

Great, I'm on board with all of this, as long as @mattsse / @Evalir also agree. Then the one nit we'll have to standardize is the naming conventions:

  • vm.eth_getLogs vs. vm.ethGetLogs
  • EthGetLogsResponse[] vs. Eth_GetLogs_Response[] vs. EthGetLogs_Response[], arguably even EthGetLogs[] works

My weakly held opinion here is vm.eth_getLogs() (to identically match the RPC name and make it clear this is an RPC query), and EthGetLogs[] but I can be convinced of other naming styles

@puma314
Copy link
Contributor Author

puma314 commented Jul 10, 2023

If we're all on board with the following, can start working on implementing this!

  • Cheatcode named: vm.eth_getLogs()
  • Response: EthGetLogs[]
  • General RPC cheatcode "escape hatch": vm.rpc(string memory name, bytes memory args) returns (bytes memory returnData)

@Evalir
Copy link
Member

Evalir commented Jul 10, 2023

I'm onboard with the naming—while i think eth_getLogs definitely breaks the pattern we've had so far, it's good to mirror the actual rpc naming. Let's do it

@Evalir Evalir marked this pull request as ready for review July 17, 2023 18:00
@Evalir
Copy link
Member

Evalir commented Jul 17, 2023

whoops @puma314 sorry—marked this as RFR even though youre still cleaning up. Feel free to ping me when you want another review round!

@puma314
Copy link
Contributor Author

puma314 commented Jul 29, 2023

@Evalir ready for another round of review! Addressed all of your existing comments for getLogs and also made .rpc(...) work with a simple test case.

@Evalir
Copy link
Member

Evalir commented Jul 31, 2023

Sweet! Do you think you can resolve the conflicts? @puma314

@puma314
Copy link
Contributor Author

puma314 commented Jul 31, 2023

Just fixed @Evalir!

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

looking good! just some more asks re docs and tests.

would love your eyes here @mattsse!

evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
testdata/cheats/Fork2.t.sol Outdated Show resolved Hide resolved
testdata/cheats/Vm.sol Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/fork.rs Outdated Show resolved Hide resolved
@Evalir Evalir force-pushed the uma/feat-get-logs branch from 8e9e768 to e8b3a79 Compare August 15, 2023 19:10
@Evalir Evalir force-pushed the uma/feat-get-logs branch from 4c29fd8 to 7f4d16b Compare August 15, 2023 20:13
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

aight, took this over the line—just needed a smol fix to the test. Thank you!

@Evalir Evalir merged commit 1b2a239 into foundry-rs:master Aug 21, 2023
@puma314
Copy link
Contributor Author

puma314 commented Aug 21, 2023

Thank you so much @Evalir!! Sorry the last few weeks got a little busy so I didn't have time to finish this up. Awesome that this is finally merged in, I'm excited to use it :D

mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
* Initial implementation

* More comprehensive test

* Added TODOs

* Test passes

* Cleaning up PR

* Tests pass

* Cleaned up get_logs, starting to work on rpc

* eth get logs should be done. still working on rpc

* RPC test works with get_balance

* Formatting

* Removed pub

* Minor solidity fixes

* Remake public

* Cheats -> vm

* chore: docs

* chore: docs

* chore: clippy

* fmt

* chore: fix path

* chore: enable permissions

* enable permissions

---------

Co-authored-by: Enrique Ortiz <[email protected]>
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.

3 participants