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(forge): vm.prompt* cheatcodes #5509

Closed
klkvr opened this issue Jul 31, 2023 · 27 comments · Fixed by #7600
Closed

Feat(forge): vm.prompt* cheatcodes #5509

klkvr opened this issue Jul 31, 2023 · 27 comments · Fixed by #7600
Assignees
Labels
T-feature Type: feature

Comments

@klkvr
Copy link
Member

klkvr commented Jul 31, 2023

Component

Forge

Describe the feature you would like

Add vm.promptAddress(string prompt), vm.promptUint(string prompt), etc. cheatcodes which would prompt user for some input. This could be used as an alternative of script function args.

Reasons why I think this could be useful:

  1. When working on a new external project it would be handy if user could just run the script and follow the instructions after it rather than checking certain function parameters and signatures in random .sol script files to enter them in cli. Also, prompt string would allow to specify more details on what needs to be provided
  2. This would allow more straightforward UI because scripts relying on some arguments will not require explicit specification of them in --sig argument

Expected behavior:

contract Deploy is Script {
    function deployFoo() public {
        address bar = vm.promptAddress("Enter bar address: ");
        new Foo(bar);
    }
}

Additional context

No response

@klkvr klkvr added the T-feature Type: feature label Jul 31, 2023
@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 5, 2023

I would also like this feature.

Personally I would find this useful also for things like choosing the chain (by passing it to vm.createSelectFork). I would also like to propose a vm.promptPassword which would present an invisible input for the user.

@mattsse WDYT about this feature request?

@mattsse
Copy link
Member

mattsse commented Oct 5, 2023

I think useful, however, this could lead to issues in CI.

so this definitely should use some timeout so it doesn't hang indefinitely

but I think this is nice to have, any concerns @mds1 ?

@mds1
Copy link
Collaborator

mds1 commented Oct 5, 2023

Partly related to #2399 (comment) and #2900 in that we currently have no script-only cheatcodes. Currently all cheatcodes can be used in both script and test environments.

I am supportive of this idea though, and you can more broadly imagine forge script interactively prompting you to choose a method and provide inputs if not provided from the CLI invocation

We should have vm.prompt* cheats for all standard types as singular values and arrays, so bool, uint, int, address, bytes32, string, bytes. The input to the cheat should be a string telling the user what to enter, for example:

  • vm.promptUint(string calldata) invoked as vm.promptUint("enter your favorite number")
  • vm.promptUintArray(string calldata) invoked as vm.promptUintArray("enter your favorite number")

Let me know if that spec sounds right to everyone

@klkvr
Copy link
Member Author

klkvr commented Oct 5, 2023

I think it makes sense to keep the format same as it's done for vm.env* cheatcodes

Such spec would look like this:

  • vm.promptUint(string calldata) returns (uint256) invoked as vm.promptUint("enter your favorite number")
  • vm.promptUint(string calldata, string delim) returns (uint256[]) invoked as vm.promptUint("enter your favorite numbers", ",")

Also, should we consider promt* cheatcodes with default values?

@mds1
Copy link
Collaborator

mds1 commented Oct 5, 2023

How does the delim arg work for string arrays in the existing method? If I have a delimiter of , and enter two strings as ab, cd, then are my two strings:

  • ab and cd, or
  • ab and cd?

My thinking was just always use comma separated and trim whitespace for strings if there's no quotes, but matching the existing env cheats is reasonable to me also.

Also, should we consider promt* cheatcodes with default values?

This seems reasonable

@mds1
Copy link
Collaborator

mds1 commented Oct 5, 2023

So the full spec then is:

  • vm.promptUint(string calldata) returns (uint256) invoked as vm.promptUint("enter your favorite number")
  • vm.promptUintOr(string calldata, uint defaultValue) returns (uint256) invoked as vm.promptUint("enter your favorite number")
  • vm.promptUint(string calldata, string delim) returns (uint256[]) invoked as vm.promptUint("enter your favorite numbers", ",")
  • vm.promptUintOr(string calldata, string delim, uint defaultValue) returns (uint256[]) invoked as vm.promptUint("enter your favorite numbers", ",")

for each of the aforementioned types?

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 5, 2023

I was actually thinking we can go the simpler route and only allow strings, since we have a complete arsenal for parsing strings (vm.parse*, vm.parseJson*). I think the processing could be left to the author.

So basically expose two methods:

function prompt(string memory prompt) returns (string memory input);
function promptSecret(string memory prompt) returns (string memory input);

If needed, we could parse arrays using vm.jsonParse*Array.

We could also start from supporting string-only, and if we see it's not enough we can add all the overloads.

@mds1
Copy link
Collaborator

mds1 commented Oct 5, 2023

Hmm interesting idea, ok I'm onboard with that newest proposal. It helps keep the API clean, and if it ends up cluttering code too much (having to manually parse the string with e.g. vm.parseUint (no json parsing cheats needed)) we can add more overloads

@klkvr
Copy link
Member Author

klkvr commented Oct 5, 2023

If there will be need in more overloads this should be possible to be done with just some new forge-std library which will look like this (or maybe we can add it from the beginning)

library StdPrompts {
     VmSafe private constant vm = VmSafe(address(uint160(uint256(keccak256("hevm cheat code")))));

     function promptUint(string memory prompt) internal pure returns (uint256) {
        return vm.parseUint(vm.prompt(prompt));
     }
    ...
}

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 5, 2023

If there will be need in more overloads this should be possible to be done with just some new forge-std library which will look like this (or maybe we can add it from the beginning)

library StdPrompts {
     VmSafe private constant vm = VmSafe(address(uint160(uint256(keccak256("hevm cheat code")))));

     function promptUint(string memory prompt) internal pure returns (uint256) {
        return vm.parseUint(vm.prompt(prompt));
     }
    ...
}

I like this idea a lot.

@mds1
Copy link
Collaborator

mds1 commented Oct 5, 2023

Instead of adding things like that to forge-std, we should just make them native cheats :) see #3782

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 5, 2023

Oh interesting, I didn't consider compilation time 😅

@klkvr
Copy link
Member Author

klkvr commented Oct 5, 2023

Yeah, #3782 seems reasonable

Adding just prompt and promptSecret seems to be enough for now. I don't think that we will need any more overloads as wrapping prompt's in parse* shouldn't clutter code too much

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 6, 2023

So if everyone is onboard, I will gladly implement this.

I will say I decided last minute to participate in ETHOnline so I will probably start working on it after the hackathon (It ends Oct 22nd I believe).

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 22, 2023

Ok I'm "back" from ETHOnline.
Three questions:

  1. Going over the thread again I recalled @mattsse's comment regarding CI hangups. If we implement a timeout, should we also implement a "default value", in case the timeout is reached (or user doesn't input anything)?
function prompt(string memory prompt, string memory default) returns (string memory input);
function promptSecret(string memory prompt, string memory default) returns (string memory input);
  1. Where would be most appropriate to add this, ext.rs?

  2. I see Cast is using dialoguer crate for prompts, I will use the same, unless anyone has any objections?

@mds1
Copy link
Collaborator

mds1 commented Oct 24, 2023

  1. A default value can be dangerous in production by resulting in the script executing with unexpected values, so we should revert on timeout
  2. There is currently a cheatcodes refactor in progress, so I’d suggest waiting for that to be completed first to avoid rework, and since this won’t get merged until the refactor is complete anyway (not sure of the current status offhand)
  3. Seems reasonable

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 24, 2023

Thank you @mds1 for the heads up 🙏 Is there a PR/issue I can subscribe to to follow the refactor, so I know when it's done?

@mds1
Copy link
Collaborator

mds1 commented Oct 24, 2023

Deferring to @mattsse @Evalir there since it’s likely split across a few PRs

@mattsse
Copy link
Member

mattsse commented Oct 25, 2023

new crate layout should be stable by EOW

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 25, 2023

Cool, thank you 👍

@Tudmotu
Copy link
Contributor

Tudmotu commented Nov 2, 2023

@mattsse @mds1 so the refactor has been completed? I see the cheatcodes were moved to their own crate?
Is there any guide explaining how to add cheatcodes with the new architecture?
If not, I'll just figure it out the old way 😄
Thanks

@DaniPopes
Copy link
Member

@Tudmotu
Copy link
Contributor

Tudmotu commented Dec 24, 2023

@Tudmotu Hey, yes it has been. You can find the docs here: https://github.com/foundry-rs/foundry/blob/master/docs/dev/cheatcodes.md#adding-a-new-cheatcode

Thank you!

Just as an update here: I've been busy with work stuff, but hopefully I will get to work on this in the coming weeks.

@Tudmotu
Copy link
Contributor

Tudmotu commented Mar 21, 2024

@klkvr should we close this issue seeing #7012 is merged?

@mattsse mattsse closed this as completed Mar 21, 2024
@mattsse mattsse reopened this Mar 21, 2024
@mattsse
Copy link
Member

mattsse commented Mar 21, 2024

@Tudmotu

I think we also want promptAddress and promptUint for convenience

@Tudmotu
Copy link
Contributor

Tudmotu commented Mar 21, 2024

Ok, yeah that's why I was not sure if we wanna close it 👍

@kamuik16
Copy link
Contributor

kamuik16 commented Apr 8, 2024

@Tudmotu

I think we also want promptAddress and promptUint for convenience

will implement this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

6 participants