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

add ffi cheatcode #185

Merged
merged 15 commits into from
Sep 12, 2023
Merged

add ffi cheatcode #185

merged 15 commits into from
Sep 12, 2023

Conversation

luksgrin
Copy link
Contributor

@luksgrin luksgrin commented Aug 18, 2023

This PR contains the implementation of Foundry's function ffi(string[] calldata) external returns (bytes memory); cheatcode with some helper functions and some code refactoring. See ffi's documentation here.

Dependencies added

  • Python's native subprocess module

@luksgrin
Copy link
Contributor Author

I am aware that both extract_string_argument and extract_string_array_argument share a lot of logic. But I did not want to risk breaking extract_string_argument for a silly refactoring...

@daejunpark
Copy link
Collaborator

daejunpark commented Aug 18, 2023

Great contribution! To help us review, could you please add tests for this cheatcode? Also, could you disable the ffi cheatcode by default (for security reasons), and add an option to enable it? Thank you!

@luksgrin
Copy link
Contributor Author

@daejunpark Please let me know if the tests I added are enough. I wanted to add one to test execution failure, but there is no expectRevert cheatcode... Possibly a # TODO?

@karmacoma-eth
Copy link
Collaborator

looks good, thanks @luksgrin! I'll make a couple small changes to have CI pass directly to the branch if you don't mind

@luksgrin
Copy link
Contributor Author

looks good, thanks @luksgrin! I'll make a couple small changes to have CI pass directly to the branch if you don't mind

No problem! Thanks😄

@karmacoma-eth karmacoma-eth enabled auto-merge (squash) August 21, 2023 17:45
@daejunpark
Copy link
Collaborator

@daejunpark Please let me know if the tests I added are enough. I wanted to add one to test execution failure, but there is no expectRevert cheatcode... Possibly a # TODO?

Awesome! Currently, you can use the expected json exitcode to test the failure cases. Please feel free to add negative tests if you already have something in mind.

@luksgrin
Copy link
Contributor Author

@daejunpark Please let me know if the tests I added are enough. I wanted to add one to test execution failure, but there is no expectRevert cheatcode... Possibly a # TODO?

Awesome! Currently, you can use the expected json exitcode to test the failure cases. Please feel free to add negative tests if you already have something in mind.

I had one but I removed it! I will write it again so that we can add it 😃

auto-merge was automatically disabled August 27, 2023 12:13

Head branch was pushed to by a user without write access

@luksgrin
Copy link
Contributor Author

@daejunpark Please let me know if the tests I added are enough. I wanted to add one to test execution failure, but there is no expectRevert cheatcode... Possibly a # TODO?

Awesome! Currently, you can use the expected json exitcode to test the failure cases. Please feel free to add negative tests if you already have something in mind.

I had one but I removed it! I will write it again so that we can add it 😃

Done!

@karmacoma-eth karmacoma-eth enabled auto-merge (squash) August 29, 2023 16:35
@daejunpark
Copy link
Collaborator

Thank you! Let me reorg the pytest tests to exclude the ffi tests on windows.

@daejunpark
Copy link
Collaborator

daejunpark commented Sep 12, 2023

@luksgrin I've modified the semantics of ffi so that it does not fail (but generates an warning) when stderr is present, in order to align with Foundry's behavior: 57241d8

Please let me know if you have a concern with this!

@daejunpark
Copy link
Collaborator

@luksgrin let me go ahead and merge this for now, as it's been open for quite some time. we can create a separate pr if the semantics of ffi needs to be improved. thanks for your contribution!

@daejunpark daejunpark merged commit 8eb41ba into a16z:main Sep 12, 2023
91 checks passed
@luksgrin
Copy link
Contributor Author

Tysm for merging! I believe it is a perfect choice to imitate foundry's default behavior :)

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