Skip to content

refactor: safer ffi function and return non hex as string #2520

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jul 31, 2022

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Jul 29, 2022

Motivation

  • make ffi safer
  • support non hex command result

Solution

  • remove dangerous unsafe call
  • add several checks (len, non empty)
  • more verbose errors

decode if output is hex otherwise return output as is

@mattsse mattsse requested review from gakonst and onbjerg July 29, 2022 23:28
@mattsse mattsse added the C-forge Command: forge label Jul 29, 2022
@tynes
Copy link
Contributor

tynes commented Jul 30, 2022

What do you think about catching stderr and also printing that if the verbosity is high enough? There were times when I wanted to debug a script I was calling from FFI and it would be nice to be able to log from the script by writing to stderr

@mattsse
Copy link
Member Author

mattsse commented Jul 30, 2022

good point, should print this if verbosity is high enough

will add

@mattsse mattsse force-pushed the matt/refactor-ffi branch from 9465806 to 64f64cf Compare July 30, 2022 15:22
.map_err(util::encode_error)?;
.map_err(|err| util::encode_error(format!("Failed to execute command: {}", err)))?;

if state.config.verbosity >= 3 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how I feel about this since it's w/o context - tests are run in parallel, so this is potentially going to be printed next to something entirely unrelated

Talked about maybe adding a CheatcodeFailed(string msg, string[] context) error type or something we could revert with instead to solve this, but that's obviously a bigger piece of work. Unsure if you want to wait with this stderr thing until then? Also removes the need to pass verbosity to the cheatcode inspector.

I also wrote about something similar a long time ago here: #928

Copy link
Member

Choose a reason for hiding this comment

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

Agree with bjerg, would recommend we skip that change here and think about it a bit more in #928

@onbjerg onbjerg added the T-feature Type: feature label Jul 30, 2022
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

lgtm modulo the bjerg comment

.map_err(util::encode_error)?;
.map_err(|err| util::encode_error(format!("Failed to execute command: {}", err)))?;

if state.config.verbosity >= 3 {
Copy link
Member

Choose a reason for hiding this comment

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

Agree with bjerg, would recommend we skip that change here and think about it a bit more in #928

@mattsse mattsse force-pushed the matt/refactor-ffi branch from 64f64cf to f44b6c7 Compare July 31, 2022 11:09
@mattsse
Copy link
Member Author

mattsse commented Jul 31, 2022

reverted,

perhaps we need some kind of Shell struct that handles printing and everything that needs to print will use that

@mattsse mattsse merged commit 9ed1c37 into foundry-rs:master Jul 31, 2022
mattsse added a commit to mattsse/foundry that referenced this pull request Aug 1, 2022
gakonst pushed a commit that referenced this pull request Aug 1, 2022
* refactor: use slashed paths

* refactor: safer ffi function and return non hex as string (#2520)

* bump ethers

* rm leftover
@0xPhaze
Copy link

0xPhaze commented Sep 1, 2022

Coming back to this, because this has hit me multiple times with a very undesirable outcome.
My use-case is that I'm using it for storage layout comparisons checks and I cannot figure out whether the script was successful and just returned nothing or it crashed. The result is to default to accepting the upgrade..

So: Is there any compelling argument to continue a script execution if ffi fails?
Why not just throw and let the user catch it if desired?

@gakonst
Copy link
Member

gakonst commented Sep 6, 2022

Exploring tryFfi as an idea in #2932

iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* refactor: use slashed paths

* refactor: safer ffi function and return non hex as string (foundry-rs#2520)

* bump ethers

* rm leftover
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants