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(cheatcodes) vm.prompt: Prompt user for interactive input #7012

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

Tudmotu
Copy link
Contributor

@Tudmotu Tudmotu commented Feb 4, 2024

Implementing #5509. I am reviving this, took me a while to get to this, apologies.
This PR will add two cheatcodes: vm.prompt() and vm.promptSecret(). These cheatcodes will let users enter dynamic values that can be consumed by Forge scripts.

The description below is long. It mostly explains the issues I encountered.
TLDR:

  • I could not implement a timeout due to limitation in Dialoguer
  • I did not write tests because I couldn't figure out how to test user input
  • Some open questions at the bottom

Motivation

It would be useful to pass arguments to scripts in a dynamic & user-friendly way. Today the options for passing arguments are either environment variables or using the --sig argument. Both of these are cumbersome and in addition, if the parameter is some secret (password, private key, api key, etc), it exposes it in the shell history.
Adding the vm.prompt() and vm.promptSecret() cheatcodes will allow script authors to write better scripts and more easily generalize their arguments. For example, scripts could prompt for a fork alias to pass to vm.createSelectFork(), or the address of a contract they interact with, etc.

Implementation

We decided to keep the interface simple and only allow string memory responses, letting the script author handle any necessary parsing using the usual string parsing methods. The implementation is based on the Dialoguer crate which is already used elsewhere and it is quite straightforward.

Timeout

Adding a timeout was suggested in order to prevent CI environments from hanging unexpectedly. These timeouts would be handled by script authors using try / catch in Solidity if desired.
Unfortunately, Dialoguer does not support timeouts for prompts. Looking at other prompt libraries it does not seem they support a timeout either. I was able to hack together a partial solution by running Dialoguer in a thread and using mpsc::channel to wait for its response up to a certain timeout. This works for Input prompts but not for Password prompts. See this comment I left on the timeout feature request on Dialoguer repo.

Therefore I decided to leave the timeout feature out of the initial PR so we can discuss other approaches (see "Open questions").

Testing

Since this cheat requires interactivity it cannot be tested in Solidity ― only in Rust. But unfortunately I couldn't figure out exactly how to test it. The main problem is that the
One option I thought we could try is mocking one of the underlying components Dialoguer uses ― Term ― in order to mock user input. But after looking at some Rust libraries it seems you cannot mock a struct from external libraries.
Another idea I had was to try and interact with the Term object via threads: run prompt in main thread, write into Term in a different thread. While visually it works (the terminal display the prompt & answer) it seems that Dialoguer does not register the input from the other thread.

Open questions

  1. For the CI hangups issue, we could explore other approaches:
    1. Use a different package which might work with the mpsc::channel solution
    2. Detect interactive environment with atty (not sure if this would actually work)
    3. Follow the --non-interactive flag forge script already has. This would require implementing a similar flag for forge test
    4. Decide to ignore this and let script authors handle timeouts externally (e.g. put a timeout on their CI job)
  2. Currently the error returned from this cheatcode is only catchable with catch (bytes memory) and not with catch Error(string memory). Is there a way to fix this?
  3. Testing ― as mentioned above, I couldn't figure out testing. Anyone got an idea?

Dependent PRs

  • Foundry Book
    • Document @klkvr suggestion as best practice for parameterized scripts running in CI
    • Open PR
  • Forge-Std

@klkvr
Copy link
Member

klkvr commented Feb 5, 2024

this looks great! thanks for implementing this

Currently the error returned from this cheatcode is only catchable with catch (bytes memory) and not with catch Error(string memory). Is there a way to fix this?

Error(string) is a selector for Solidity errors coming from revert/require and only such errors are catchable by catch Error(string). Cheatcode errors are encoded as a CheatcodeError(string), so it's fine and expected that they can only be catched via catch (bytes)

I was able to hack together a partial solution by running Dialoguer in a thread and using mpsc::channel to wait for its response up to a certain timeout. This works for Input prompts but not for Password prompts.

Your code from console-rs/dialoguer#266 (comment) with some small modifications seems to be working for me

fn prompt_secret(prompt_text: String) -> Result<String> {
    let (send, recv) = mpsc::channel();

    thread::spawn(move || {
        send.send(Password::new().with_prompt(prompt_text).interact()).unwrap();
    });

    match recv.recv_timeout(Duration::from_secs(5)) {
        Ok(res) => res.map_err(|_| "i/o error occured".into()),
        Err(_) => Err("timeout".into())
    }
}

However, there were several times when it didn't react on any input and failed with timeout, but this looked like a shell bug and simple restart of the shell fixed it. I can't reliably reproduce this behavior.

Testing ― as mentioned above, I couldn't figure out testing. Anyone got an idea?

I can imagine some kind of InputSource abstraction which would be a wrapper around Dialoguer usually and which we can mock in tests. I think @DaniPopes or @mattsse might have some better ideas

Let's also hide values received from promptSecret in traces as it's done with env variables here:

fn decode_cheatcode_outputs(&self, func: &Function) -> Option<String> {
match func.name.as_str() {
s if s.starts_with("env") => Some("<env var value>"),
"deriveKey" => Some("<pk>"),
"parseJson" if self.verbosity < 5 => Some("<encoded JSON value>"),
"readFile" if self.verbosity < 5 => Some("<file>"),
_ => None,
}
.map(Into::into)
}

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 5, 2024

Thank you @klkvr for the quick response! 🙏

  1. Regarding the error: Ok, got it, thanks 🙂
  2. Regarding the timeout: that's awesome, I will try it out! Interesting what you're saying about a shell bug. I am using tmux, so might be related? I will test it out and report back
  3. Regarding testing, are you suggesting a sort of DI? Let me know if I understand your suggestion correctly:
    1. Create a new struct, InputSource, which implements the same API as Dialoguer but with traits, and delegates the function calls to an internal instance of Dialoguer
    2. Change prompt and prompt_secret so they accept an instance of this new trait
    3. In the tests module, create MockInputSource which implements the trait but does not actually interact with the terminal, simply returns a hard-coded value
  4. Regarding hiding values in traces: good catch! I will do that

@klkvr
Copy link
Member

klkvr commented Feb 5, 2024

  1. Yeah, I meant DI approach here, however not sure if it really worth the effort (we would have to somehow pass the mocked InputSource all the way down to the Cheatcodes when running tests), as we won't actually test any stdin interactions that way.

Let's figure out how to deal with timeouts first, and after that we can add tests for timeouts causing reverts which seems to be more important and won't require any mocking

@mds1
Copy link
Collaborator

mds1 commented Feb 5, 2024

One thing we also should figure out is how can scripts be used in a non-interactive/test environment with this cheat. It's best practice to test scripts, and many use scripts to setup their test state by running the script in setUp. I think the value of this cheat goes down significantly compared to a confirmContinue approach if this cannot be used in a test environment.

One approach might to add two extra args: the default value, and a bool of whether or not to immediately use that value (i.e. no timeout). This is also nice because the default arg gives us stronger return typing for free. This would fit really nicely with #2900 to remove the need for an env var. Examples

// new cheat signature
prompt(string calldata promptText, uint256 default, bool useDefault) external returns (uint256 value);

// usage with env var config
uint256 myUint = vm.prompt("enter uint", 42, vm.envOr("TEST_MODE", false));

// with forge environment detection
uint256 myUint = vm.prompt("enter uint", 42, vm.isTestContext());

@klkvr
Copy link
Member

klkvr commented Feb 5, 2024

@mds1 Good point. IMO, having a single default value which is used in test environment will not always be enough. I believe that for testing scripts with prompt cheats such pattern would make sense:

contract Script {
    function run() public {
        uint256 myUint = vm.parseUint(vm.prompt("enter uint"));
        run(myUint);
    }

    function run(uint256 myUint) public {
        // actual logic
    }
}

That way, we are keeping UX gain (don't have to provide --sig argument when running script), but tests can set any value to myUint and not just hardcoded default.

@Tudmotu Tudmotu force-pushed the tudmotu/vm.prompt branch 2 times, most recently from 0bcee87 to e7cbaa0 Compare February 5, 2024 16:31
@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 5, 2024

@mds1 would you like me to implement #2399 as part of this PR? Should be simple enough I think. Dialoguer supports a confirm-type prompt:
https://docs.rs/dialoguer/latest/dialoguer/struct.Confirm.html

Regarding a "default value" ― script authors could catch the timeout and provide a default for themselves:

function promptWithDefault (
    string memory prompt,
    string memory defaultValue
) internal returns (string memory input) {
    try vm.prompt(prompt) returns (string memory res) {
        if (bytes(res).length == 0) input = defaultValue;
        else input = res;
    }
    catch (bytes memory) {
        input = defaultValue;
    }
}

Not sure if that truly answers your concerns, but something to consider.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 5, 2024

I'm still working on the timeout, I added it in the config but it seems I broke some tests. Looking into it.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 5, 2024

@klkvr @mds1 @DaniPopes I see I have tests that are failing. I am very certain there is an actual issue because the original commit for the PR fully passed the tests.
When I try to run the tests locally using e.g.:

$ cargo nextest run -E 'kind(test) & !test(/issue|forge_std|ext_integration/)' --partition count:2/3

I get different tests failing 🤔 (specifically one forge::cli cmd::can_install_missing_deps_build).

One noticeable difference is that it seems these tests are running on Windows while my machine is Linux.

What would be the best way to try and reproduce these failures locally?

@mds1
Copy link
Collaborator

mds1 commented Feb 5, 2024

@mds1 would you like me to implement #2399 as part of this PR? Should be simple enough I think. Dialoguer supports a confirm-type prompt:
https://docs.rs/dialoguer/latest/dialoguer/struct.Confirm.html

I would do this as a separate PR, just to manage scope, and ensure one feature doesn't block the other

@mds1 Good point. IMO, having a single default value which is used in test environment will not always be enough. I believe that for testing scripts with prompt cheats such pattern would make sense:

contract Script {
    function run() public {
        uint256 myUint = vm.parseUint(vm.prompt("enter uint"));
        run(myUint);
    }

    function run(uint256 myUint) public {
        // actual logic
    }
}

That way, we are keeping UX gain (don't have to provide --sig argument when running script), but tests can set any value to myUint and not just hardcoded default.

This is a good idea, I'd be ok with this as the solution, and we should make sure to document this pattern in the book. Let's go with your workflow here to keep things simpler, as my suggestion can always be implemented in the future in a backwards compatible way

@DaniPopes
Copy link
Member

I see I have tests that are failing

@Tudmotu Some tests are very flaky unfortunately. I've re-ran the failing jobs and they pass now.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 5, 2024

I see I have tests that are failing

@Tudmotu Some tests are very flaky unfortunately. I've re-ran the failing jobs and they pass now.

Ah, thank you @DaniPopes 🙏
I was pretty certain I broke something because my local tests were also failing, albeit other ones. But I guess for some other reason.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 6, 2024

Looking at my code I am pretty sure I did not implement the config option correctly. It works, but it feels wrong to modify evm/core for this, right?

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 18, 2024

I pushed another change that removes the unnecessary changes I made to evm/core.

I would like to push this PR forward.

Regarding tests:
Would we like to follow the approach suggested by @klkvr?
I describe it here: #7012 (comment)

@klkvr
Copy link
Member

klkvr commented Feb 22, 2024

I think it's fine to just have a couple tests like

vm._expectCheatcodeRevert("Prompt timed out");
vm.prompt(...);

vm._expectCheatcodeRevert("Prompt timed out");
vm.promptSecret(...);

we have default timeout set to 0 for testing, so it shouldn't cause any issues with our CI

@mattsse wdyt?

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 23, 2024

It seems using vm.prompt fails with a different error because Dialoguer detects it is not running in an interactive shell:
image

Looking into how to make it work.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Feb 24, 2024

After looking into it, it seems to be caused by nextest capturing stdout. Running with --no-capture allows Dialoguer to run but it hangs for some reason. The hangup might be solvable, but I'm more uncertain about the --no-capture flag, since it requires running the tests sequentially.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 11, 2024

Hi all,
I would like to get a temp check on this. Is there anything I can do to push this feature forward?

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.

sorry for the late review...

I have only a few nits.

I'm fine with not having tests for this, because not really possible without doing a lot of work,
but you're right this will fail with "not a terminal" if there's no terminal

I like the test case for prompttimeout that @klkvr suggested, this one we can add easily

we could add another cheatcode for is_terminal, but can do this seprately

@@ -370,6 +387,39 @@ fn ffi(state: &Cheatcodes, input: &[String]) -> Result<FfiResult> {
})
}

fn prompt_input(prompt_text: &String) -> Result<String, dialoguer::Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn prompt_input(prompt_text: &String) -> Result<String, dialoguer::Error> {
fn prompt_input(prompt_text: &str) -> Result<String, dialoguer::Error> {

Input::new().allow_empty(true).with_prompt(prompt_text).interact_text()
}

fn prompt_password(prompt_text: &String) -> Result<String, dialoguer::Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn prompt_password(prompt_text: &String) -> Result<String, dialoguer::Error> {
fn prompt_password(prompt_text: &str) -> Result<String, dialoguer::Error> {

Comment on lines 413 to 414
println!();
"I/O error occured".into()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should return the error as the return value and also log in in an error! trace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand - do you mean the prompt() function should return Result<String, dialoguer::Error>?

Copy link
Member

Choose a reason for hiding this comment

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

we want to return err.to_string() here as the response value so the users has a chance to look at the error in the revert message if the call failed

fn prompt(
state: &Cheatcodes,
prompt_text: &str,
input: fn(&String) -> Result<String, dialoguer::Error>,
Copy link
Member

Choose a reason for hiding this comment

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

&str

fn prompt(
state: &Cheatcodes,
prompt_text: &str,
input: fn(&String) -> Result<String, dialoguer::Error>,
Copy link
Member

Choose a reason for hiding this comment

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

&str

@Tudmotu Tudmotu force-pushed the tudmotu/vm.prompt branch 2 times, most recently from ba54c2d to d3dae42 Compare March 20, 2024 19:45
@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 20, 2024

Pushed a change which propagates the error message.
Also added tests that make sure this error exists.

Will prepare PRs for Foundry Book & Forge Std.

Let me know if there is anything else you'd like me to fix here.

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 20, 2024

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 after addressing outstanding comments, feel free to open the PRs

crates/cheatcodes/src/config.rs Outdated Show resolved Hide resolved
@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 21, 2024

Whoops, I was certain I already made these changes but I guess I messed something up with my rebases 😅

I believe everything should be addressed now 🙂

@mattsse mattsse marked this pull request as ready for review March 21, 2024 12:34
@mattsse mattsse requested a review from klkvr as a code owner March 21, 2024 12:34
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,

I set prompt timeout to 0 for testing and use a regular revert, so this is still usable when running tests locally

@mattsse
Copy link
Member

mattsse commented Mar 21, 2024

@Tudmotu

we can now add it to forge-std

@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 21, 2024

Thank you @mattsse 🙏

I will open the Book & Std PRs soon.

@mattsse mattsse merged commit b342ff2 into foundry-rs:master Mar 21, 2024
19 of 20 checks passed
@Tudmotu
Copy link
Contributor Author

Tudmotu commented Mar 21, 2024

Thank you all for the review, effort and patience 🙂 🙏

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.

5 participants