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(traces): show state changes in cast run and forge test on -vvvvv #9013

Conversation

cassc
Copy link
Contributor

@cassc cassc commented Oct 3, 2024

Motivation

The current human-readable output from cast run is great for manual interpretation but requires additional parsing to be used programmatically. Additionally, there is a need to inspect storage changes for each call, but currently, these changes are not recorded.

Solution

This PR introduces a -j option for cast run to output traces in JSON format, making it easier to consume programmatically. It also adds a --with-state-changes option that outputs storage changes.

Example usage:

cast run 0xefc789b63631b255aeb6f97d948c9eac14bae4b7f021122fe24c7c6e4f34667f \
  -r https://eth.llamarpc.com -q --decode-internal --with-state-changes -j | \
  jq -R 'select(try fromjson? | . != null) | fromjson'

@cassc cassc marked this pull request as ready for review October 3, 2024 08:58
Copy link
Member

@zerosnacks zerosnacks 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 your PR, generally in favor of adding this

crates/verify/src/utils.rs Outdated Show resolved Hide resolved
@cassc cassc requested a review from zerosnacks October 8, 2024 02:34
@cassc
Copy link
Contributor Author

cassc commented Oct 16, 2024

Hi everyone, please let me know if there's anything else I can do to help move this PR forward.

mattsse pushed a commit to paradigmxyz/revm-inspectors that referenced this pull request Oct 16, 2024
This PR writes the storage change in the trace. Example output when
using cast run from [the
PR](foundry-rs/foundry#9013):

```
❯ cargo run --bin cast run 0xefc789b63631b255aeb6f97d948c9eac14bae4b7f021122fe24c7c6e4f34667f   -r https://eth.llamarpc.com  -q --decode-internal --with-state-changes

Compiling: TransparentUpgradeableProxy 0x6bE457e04092B28865E0cBa84E3b2CFa0f871E67
Compiling: TransparentUpgradeableProxy 0x7a7f0b3c23C23a31cFcb0c44709be70d4D545c6e
Compiling: TransparentUpgradeableProxy 0xD523794C879D9eC028960a231F866758e405bE34
Compiling: Pool 0xDEbbf61098642C7c06fAd1E116C1a00e50405D0d
Traces:
  [247473] TransparentUpgradeableProxy::stake{value: 1949677189193480698}(1)
    ├─ [2232] TransparentUpgradeableProxy::_beforeFallback()
    │   ├─ [2150] ERC1967Upgrade::_getAdmin()
    │   │   └─ ← 0xD491302a6621128f4b5a6A49ce6657230732b0cb
    │   └─ ←
    ├─ [2196] ERC1967Proxy::_implementation()
    │   └─ ← 0xDEbbf61098642C7c06fAd1E116C1a00e50405D0d
    ├─ [240157] Pool::stake{value: 1949677189193480698}(1) [delegatecall]
    │   ├─ [213932] Pool::_stake(0x93386C72aa57082820Ad6Aa29998B820971a8d61, 1949677189193480698 [1.949e18])
    │   │   ├─ [228681] TransparentUpgradeableProxy::deposit(0x93386C72aa57082820Ad6Aa29998B820971a8d61, 1949677189193480698 [1.949e18])
    │   │   │   ├─ [2232] TransparentUpgradeableProxy::_beforeFallback()
    │   │   │   │   ├─ [2150] ERC1967Upgrade::_getAdmin()
    │   │   │   │   │   └─ ← 0xD491302a6621128f4b5a6A49ce6657230732b0cb
    │   │   │   │   └─ ←
    │   │   │   ├─ [2196] ERC1967Proxy::_implementation()
    │   │   │   │   └─ ← 0x1a5b89b2ef0028A059EAD7D9E648B533f87c8558
    │   │   │   ├─ [221356] 0x1a5b89b2ef0028A059EAD7D9E648B533f87c8558::deposit(0x93386C72aa57082820Ad6Aa29998B820971a8d61, 1949677189193480698 [1.949e18]) [delegatecall]
    │   │   │   │   ├─ emit Update(: 1305166715630598691 [1.305e18], : 17341785000000000 [1.734e16], : 1926865000000000 [1.926e15])
    │   │   │   │   ├─ [22215] TransparentUpgradeableProxy::reStake(17341785000000000 [1.734e16], 0)
    │   │   │   │   │   ├─ [2232] TransparentUpgradeableProxy::_beforeFallback()
    │   │   │   │   │   │   ├─ [2150] ERC1967Upgrade::_getAdmin()
    │   │   │   │   │   │   │   └─ ← 0xD491302a6621128f4b5a6A49ce6657230732b0cb
    │   │   │   │   │   │   └─ ←
    │   │   │   │   │   ├─ [2196] ERC1967Proxy::_implementation()
    │   │   │   │   │   │   └─ ← 0xbE3366a14D0c87094DEB6DFbA667299b4EaC489d
    │   │   │   │   │   ├─ [14896] 0xbE3366a14D0c87094DEB6DFbA667299b4EaC489d::reStake(17341785000000000 [1.734e16], 0) [delegatecall]
    │   │   │   │   │   │   ├─ [3266] TransparentUpgradeableProxy::restake{value: 17341785000000000}(0)
    │   │   │   │   │   │   │   ├─ [232] TransparentUpgradeableProxy::_beforeFallback()
    │   │   │   │   │   │   │   │   ├─ [150] ERC1967Upgrade::_getAdmin()
    │   │   │   │   │   │   │   │   │   └─ ← 0xD491302a6621128f4b5a6A49ce6657230732b0cb
    │   │   │   │   │   │   │   │   └─ ←
    │   │   │   │   │   │   │   ├─ [196] ERC1967Proxy::_implementation()
    │   │   │   │   │   │   │   │   └─ ← 0xDEbbf61098642C7c06fAd1E116C1a00e50405D0d
    │   │   │   │   │   │   │   ├─ [2450] Pool::restake{value: 17341785000000000}(0) [delegatecall]
    │   │   │   │   │   │   │   │   └─ ← [Stop]
    │   │   │   │   │   │   │   └─ ← [Return]
    │   │   │   │   │   │   └─ ← [Stop]
    │   │   │   │   │   └─ ← [Return]
    │   │   │   │   ├─  emit topic 0: 0xb0ec6c271a891e04f03d43bf454839acfbf574e269cb5599c1f70b9257cf9cd7
    │   │   │   │   │           data: 0x000000000000000000000000000000000000000000000000003d9c42f6b23a00
    │   │   │   │   ├─  emit topic 0: 0xc8724ec5e59eea00f3f35419c3139ead03ff07766e7e9cf00a62381692aac8c7
    │   │   │   │   │        topic 1: 0x00000000000000000000000093386c72aa57082820ad6aa29998b820971a8d61
    │   │   │   │   │           data: 0x0000000000000000000000000000000000000000000000001b0ea512e08f39fa
    │   │   │   │   ├─  storage write 0x7a7f0b3c23C23a31cFcb0c44709be70d4D545c6e [0x501d093c0316d001addb7dc8913312112ff29225e2343de5aed242436c83ecaf]:
    │   │   │   │   │                 0x0000000000000000000000000000000000000000000000000000000000000000 → 0x0000000000000000000000000000000000000000000000001b0ea512e08f39fa
...
    │   │   │   │   │                 0x0000000000000000000000000000000000000000000000000000000000000003 → 0x0000000000000000000000000000000000000000000000000000000000000004
    │   │   │   │   ├─  storage write 0x7a7f0b3c23C23a31cFcb0c44709be70d4D545c6e [0xb4c87350b3618bf3b0453372aae234908cc3b6cf61d458f195edd9160dac1f7f]:
    │   │   │   │   │                 0x000000000000000000000000000000000000000000000000ce8cbfb644313c26 → 0x000000000000000000000000000000000000000000000000e9d9010c1b72b020
    │   │   │   │   └─ ← [Return] 0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
    │   │   │   └─ ← [Return] 0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
    │   │   └─ ←
    │   ├─ emit StakeAdded(staker: 0x93386C72aa57082820Ad6Aa29998B820971a8d61, value: 1949677189193480698 [1.949e18], source: 1)
    │   └─ ← [Stop]
    └─ ← [Return]


Transaction successfully executed.
Gas used: 248777
```

---------

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@cassc
Copy link
Contributor Author

cassc commented Oct 18, 2024

For the CI error:

error: this function has too many arguments (9/7)
   --> crates/cli/src/utils/cmd.rs:357:1
    |
357 | / pub async fn handle_traces(
358 | |     mut result: TraceResult,
359 | |     config: &Config,
360 | |     chain: Option<Chain>,
...   |
366 | |     with_state_changes: bool,
367 | | ) -> Result<()> {
    | |_______________^

I believe we have a few options to address this:

  1. Move the options debug, decode_internal, verbose, json, and with_state_changes into the Config struct.
  2. Suppress the warning by adding #[allow(clippy::too_many_arguments)] to this function.
  3. Extract the above 5 options into a new struct.

Please let me know your thoughts or suggestions.

@grandizzy
Copy link
Collaborator

  1. Suppress the warning by adding #[allow(clippy::too_many_arguments)] to this function.

this should be fine IMO

Cargo.toml Outdated Show resolved Hide resolved
@zerosnacks
Copy link
Member

Any updates on this PR? It's a bit time consuming to keep merging updates on the main branch. Thank you!

Hey @cassc sorry for the delay, some small notes in regards to the global --json and global -v (verbosity) flag introduced recently. I don't think there are blockers beyond that.

@zerosnacks zerosnacks self-assigned this Nov 18, 2024
@cassc cassc requested a review from zerosnacks November 19, 2024 02:57
@grandizzy
Copy link
Collaborator

grandizzy commented Nov 21, 2024

@cassc could you please check #2846 (comment) would like to know if we could tackle all points in the PR (or as a follow-up). Thank you!

@grandizzy
Copy link
Collaborator

also wonder if we need the new --with-state-changes arg or we could just use verbosity for displaying such, @zerosnacks wdyt? thanks

@cassc
Copy link
Contributor Author

cassc commented Nov 25, 2024

@cassc could you please check #2846 (comment) would like to know if we could tackle all points in the PR (or as a follow-up). Thank you!

Hi @grandizzy I think it's better to tackle in another PR, especially I think at the memoment I might not have enough knowledge about the test-level state changes.

@grandizzy
Copy link
Collaborator

grandizzy commented Nov 25, 2024

@cassc could you please check #2846 (comment) would like to know if we could tackle all points in the PR (or as a follow-up). Thank you!

Hi @grandizzy I think it's better to tackle in another PR, especially I think at the memoment I might not have enough knowledge about the test-level state changes.

make sense! @mds1 could you pls chime in re UX for cast run point in #2846 (comment) ? (that is adding the --with-state-changes arg to cast run and the way storage diffs are displayed). Then we can extend it (in a follow up PR) to forge test and add the cheatcode as suggested in 2nd point of your comment

Comment on lines 53 to 56
/// Prints the state changes
#[arg(long)]
with_state_changes: bool,

Copy link
Member

@zerosnacks zerosnacks Nov 27, 2024

Choose a reason for hiding this comment

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

Instead of introducing a new arg I would prefer we use the global verbosity (-vvv) flag here

It is accessible through shell::verbosity()

@zerosnacks zerosnacks self-requested a review November 27, 2024 11:01
@grandizzy grandizzy force-pushed the add-options-to-output-storage-change-and-json-output-for-cast-run branch from a2af542 to dc85075 Compare November 27, 2024 11:25
@grandizzy grandizzy changed the title Add options for state changes output and json output in cast run command feat(traces): show state changes in cast run and forge test on -vvvvv Nov 27, 2024
@grandizzy grandizzy changed the title feat(traces): show state changes in cast run and forge test on -vvvvv feat(traces): show state changes in cast run and forge test on -vvvvv Nov 27, 2024
@grandizzy grandizzy force-pushed the add-options-to-output-storage-change-and-json-output-for-cast-run branch from dc85075 to 85f9571 Compare November 27, 2024 11:37
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@grandizzy
Copy link
Collaborator

this should be good to go but probably worth one more reviewer since @zerosnacks and I were also adding code to it

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, left suggestion for @zerosnacks

self
}
}

pub fn with_verbosity(self, verbosiy: u8) -> Self {
if verbosiy >= 3 {
Copy link
Member

Choose a reason for hiding this comment

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

one suggestion for separate improvement @zerosnacks
let's introduce some enum for these numbers

Copy link
Member

Choose a reason for hiding this comment

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

added a ticket for it here: #9426

@mattsse mattsse merged commit c63aba8 into foundry-rs:master Nov 27, 2024
22 checks passed
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
…`-vvvvv` (foundry-rs#9013)

* Add options for state changes output and json output in cast run command

* fix test

* add back serde_json in Cargo.lock

* format using nightly

* rename parameter

* update revm-inspectors

* supress clippy warning and merge master

* add serde_json

* disable some stdout print when --json option is used

* remove unnecessary check

* replace with sh_println

* replace with shell::is_json

* Show storage for verbosity > 1, add test

* Change verbosity to > 4 for both cast and forge test, add test, fix ci

---------

Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
@grandizzy grandizzy added T-feature Type: feature C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

5 participants