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: broadcastRawTransaction cheatcode #4931

Merged
merged 23 commits into from
Jul 26, 2024

Conversation

teddav
Copy link
Contributor

@teddav teddav commented May 12, 2023

Motivation

Add a cheatcode to apply a raw signed transaction to the current state.
Closes #4816

I'm going to add some unit tests. In the meantime, @wighawag let me know if your tests run correctly.

Thanks @mattsse for the help 😊

@wighawag
Copy link

Hey, I just tried it and got the following error:

    │   ├─ [0] VM::sendRawTransaction(0xf8a58085174876e800830186a08080b853604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf31ba02222222222222222222222222222222222222222222222222222222222222222a02222222222222222222222222222222222222222222222222222222222222222) 
    │   │   └─ ← "transact_from_tx: No `to` field found"
    │   └─ ← "transact_from_tx: No `to` field found"
    └─ ← "transact_from_tx: No `to` field found"

the tx I am sending is a create tx so the to is the zero address, I guess the current PR do not support that ?

@wighawag
Copy link

By the way I am also wondering how it behave in a script with --broadcast

I am assuming it will be broadcasted and show up in the broadcast files as this is my use case.

But maybe there are other use-case that want the broadcast to happen only if vm.broadcast was called. If so it will have to ignore the broadcast sender address obviously

Interested to know what is the plan here

Evalir
Evalir previously requested changes May 17, 2023
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.

not a full review as still waiting for tests—but just a few nits upfront :)

evm/src/executor/backend/mod.rs Outdated Show resolved Hide resolved
evm/src/executor/backend/mod.rs Outdated Show resolved Hide resolved
evm/src/executor/backend/mod.rs Outdated Show resolved Hide resolved
@teddav
Copy link
Contributor Author

teddav commented May 24, 2023

Thanks for the review @Evalir !
I think I mostly managed to make everything work, but I still have an issue.

You can see it here: https://github.com/foundry-rs/foundry/pull/4931/files#diff-adc09d8ab1c41936c5b0de46796293527cd70c1a67dc170b44030830658b2053R107
In the "test_execute_signed_tx_with_revert" test. If I use sendRawTransaction, and then there is a revert, Foundry then panics when trying to revert the state. It seems to be due to the JournaledState journal, because the revert happens here:
revm/src/journaled_state.rs:283

Maybe I'm not applying the state correctly here: https://github.com/foundry-rs/foundry/pull/4931/files#diff-eeb83a97daa4af74bebc0c8b78fc406a8697412e50f5e203a116f67fb3239f23R1195

Otherwise, most tests seem to work.

@teddav
Copy link
Contributor Author

teddav commented Jun 12, 2023

the branch was getting a bit old, so i just rebased on master

@Evalir
Copy link
Member

Evalir commented Jun 12, 2023

thank you! ack. Will review this week

@wighawag
Copy link

I just tried the PR and when the sendRawTransaction is executed I get

The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: /home/wighawag/.cargo/git/checkouts/revm-bf744d8ffabcbad9/8833792/crates/revm/src/journaled_state.rs:307

This is a bug. Consider reporting it at https://github.com/foundry-rs/foundry

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   1: __libc_start_call_main<unknown>
      at ./csu/../sysdeps/nptl/libc_start_call_main.h:58
   2: __libc_start_main_impl<unknown>
      at ./csu/../csu/libc-start.c:392

This is executed with

vm.sendRawTransaction(
                        hex"f8a58085174876e800830186a08080b853604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf31ba02222222222222222222222222222222222222222222222222222222222222222a02222222222222222222222222222222222222222222222222222222222222222"
                    );

And before that call I make sure 0x3fAB184622Dc19b6109349B94811493BF2a45362 have enough balance to execute the tx (10000000 gwei)

@Evalir Evalir requested review from Evalir and mattsse June 19, 2023 12:08
@teddav
Copy link
Contributor Author

teddav commented Jun 20, 2023

That's weird @wighawag it doesnt crash for me, and I cant reproduce a crash.
The test does fail though if you don't run a node because Foundry automatically deploys create2factory in tests. I get a NonceTooLow { tx: 0, state: 1 } error.
So I'm just running an anvil node (no other options), and my script as

$ ./myforge script ./script/RawTx.s.sol -f http://127.0.0.1:8545 -vvvvv --broadcast

can you give me more details?
lmk if you want me to send the entire script

@wighawag
Copy link

I was using a complex setup so I recreated my use case in this repo :https://github.com/bug-reproduction/hello_foundry_4931

The README explains the issue that my complex setup was probably hitting

Basicaly it seems sendRawTransaction is indeed working but it does not affect the current execution environment and everything act like it was never executed, until you execute a new script

@teddav
Copy link
Contributor Author

teddav commented Jul 9, 2023

@wighawag this should be fixed. sorry for taking so long.
it was a stupid mistake, the transaction was always considered as a Call whatever the to field was. Now it's a Create if to==0 otherwise it's a Call
@Evalir @mattsse this should be good for review

@tynes
Copy link
Contributor

tynes commented Sep 10, 2023

I would definitely find this cheatcode useful

@sam-goldman
Copy link

I would definitely find this cheatcode useful

Same!

@LeoPapais
Copy link

Hey @teddav I'd like to contribute to this PR, bc it would be super useful for me. Would you be kind and provide a status update of the current code? What's in need of a fix, or any challenge that you are facing?

Thanks in advance

@teddav
Copy link
Contributor Author

teddav commented Oct 27, 2023

Hey @LeoPapais ! I'm glad you're interested in it. I am too ahah 😁
No challenge, the cheatcode is ready and working.
Unfortunately, it was never reviewed a few months back when I worked on it 🥲
And I asked again @Evalir a week ago who told me they're not merging new PRs at the moment because there is a big refactor coming (if i understood correctly).
I see some other people showed interest, and I'd love to see it merged too.
Everything was working properly, and I also added some tests. So it's just a matter of code quality review in my opinion before it's ready to be merged.
But I have to admit i'll probably be too lazy to rebase everything after the refactoring.

In the meantime, if you need it you can just pull the PR and build Foundry from there if you need this cheatcode. That's what I did. It's not that convenient though... 😉

@Evalir
Copy link
Member

Evalir commented Oct 28, 2023

Hey hey! So adding a bit more context on this:

  • Right now we need to merge a big cheatcodes refactor before we can merge new cheatcodes. This will be done soon.
  • As soon as this happens, we'll process the cheatcode backlog, including this one—it'll need a rebase/reimpl but we'll probably take care of that, as the process for creating new cheatcodes will have changed

appreciate the patience 🙏

@DaniPopes
Copy link
Member

As mentioned above by @Evalir, we recently refactored how cheatcodes are implemented in #6131. You can read more about it in the dev docs.

@spacesailor24
Copy link

Hi @DaniPopes, for my clarification, this cheatcode would need to be refactored based on #6131 in order to be merged?

@wighawag
Copy link

wighawag commented May 3, 2024

Just wondering if there is any update on this feature ?

@teddav
Copy link
Contributor Author

teddav commented May 3, 2024

i forgot about that! you come at the right time @wighawag , i might have time next week to finish that. It will be a good way for me to look at the new Foundry file structure (i havent looked into it in months).
but i admit that if it takes me too long to refactor i might be lazy... i'll try to take care of it on monday.
@tynes i saw you mentioned it could be useful for you. did you end up using it?

@DaniPopes
Copy link
Member

If you end up picking this up again, I'd recommend just starting over in a new PR rather than trying to rebase this one. Quite literally every file touched here has been completely re-written.

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm, pending @klkvr

@zerosnacks zerosnacks requested a review from klkvr July 25, 2024 15:17
@klkvr klkvr enabled auto-merge (squash) July 26, 2024 07:37
@DaniPopes DaniPopes requested review from Evalir and removed request for Evalir July 26, 2024 07:54
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.

last nits

crates/cheatcodes/spec/src/vm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks requested review from Evalir and removed request for Evalir July 26, 2024 08:45
@zerosnacks zerosnacks disabled auto-merge July 26, 2024 08:45
@zerosnacks zerosnacks enabled auto-merge (squash) July 26, 2024 08:45
@zerosnacks zerosnacks requested review from Evalir and removed request for Evalir July 26, 2024 08:45
@zerosnacks zerosnacks merged commit 1f9b52c into foundry-rs:master Jul 26, 2024
21 checks passed
@zerosnacks zerosnacks changed the title feat: sendRawTransaction cheatcode feat: broadcastRawTransaction cheatcode Jul 26, 2024
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.

feat (cheatcode) send raw signed tx