foundry icon indicating copy to clipboard operation
foundry copied to clipboard

feat: `vm.confirmContinue` cheat code

Open mds1 opened this issue 3 years ago • 11 comments

Component

Forge

Describe the feature you would like

// Prints the provided string to the console, prompts the user to enter Y/N to
// continue execution, and reverts if N is entered, (infoString) => ()
vm.confirmContinue(string calldata info) external;

This cheatcode prints the full string entered and asks you to continue with a Y/N prompt, and reverts if you enter N. The use case here is to confirm deploy parameters in a script in a table or similar before executing the script.

The vm.toString() cheatcodes along with string.concat() make it simple to put the string together. It should support things like \n, or some other common syntax used for aligning lines such as rust's format! macro.

The benefit to making this a cheatcode instead of putting it in forge-std is to avoid requiring ffi for it.

Additional context

No response

mds1 avatar Jul 20 '22 02:07 mds1

Unclear how this should work with forge test since all of them run in parallel and we'd have to block the execution... Makes it super hard to test deploy scritps that use the cheatcode, no?

onbjerg avatar Jul 20 '22 07:07 onbjerg

That's a good point. And now that I think about it, I don't think doing this with ffi would since ffi doesn't open/expose a shell for you.

What if we scope the cheatcode to only work when invoked with forge script? I think that is the best solution, since you don't want/need this in tests, and it's only useful for scripts anyway (though from an implementation perspective I'm not sure how feasible that is).

mds1 avatar Jul 21 '22 19:07 mds1

It's feasible-ish, but still some edge cases with forge script now that I think about it, e.g. when using the debugger it should also not work etc. Don't have a general stance on whether the cheatcode is good/useful so will leave that up for discussion, just trying to map out edge cases :cowboy_hat_face:

onbjerg avatar Jul 21 '22 19:07 onbjerg

True good call. Personally this is something we always add to our deploy scripts to add assurances around the deploy parameters, and just the other week we deployed something via forge script with the wrong params partly because we did not have something like this.

But happy to hear other opinions / alternative ideas as well and will let others chime in 🙂

mds1 avatar Jul 21 '22 19:07 mds1

@mds1 Can we not achieve the same result by having the user confirm before broadcast instead? Possibly also using decoded transaction data?

onbjerg avatar Aug 11 '22 19:08 onbjerg

One downside is that if you have e.g. a bug in the config for tx 2, you already sent tx 1 by the time you notice.

But in general I think it's a good idea. Maybe an upfront summary of all decoded txs, like a partial trace, where only the top-level calls of broadcasted txs are shown (or two levels deep for create2 to show the actual contract creation)

mds1 avatar Aug 11 '22 19:08 mds1

@mds1 We already have all of the transactions before we broadcast them, so my idea was that we would display them all decoded so you can inspect them if you want before broadcasting any of them?

onbjerg avatar Aug 11 '22 20:08 onbjerg

Cool, yea I like that idea. So I imagine the UX being something like:

  • A new flag to enable/disable the "confirm continue" terminal prompt
  • If enabled, show the decoded txs, perhaps in a view that looks similar to the tx traces? (I'm mostly indifferent as to the actual format here)
    • For normal txs we can decode like usual.
    • For create we'd want to decode contract name and constructor args.
    • For create2 txs, we'll need to show one level deeper (since top-level calldata is a tx to the create2 deployer, which then makes the create2 call).

Does that line up with what you're thinking?

mds1 avatar Aug 11 '22 20:08 mds1

Yep!

onbjerg avatar Aug 15 '22 18:08 onbjerg

Hey, wondering if there's been any progress on this issue. If no one is actively working/planning to work on this, I could take a crack at it, though this would be my first issue in Foundry so a code pointer or two would be much appreciated.

Dendrimer avatar Sep 19 '22 16:09 Dendrimer

Feel free to take a stab at it :)

Let me know if you end up not being able to complete it and I will unassign you!

onbjerg avatar Sep 19 '22 17:09 onbjerg

@Dendrimer I unassigned for you now, but if you do still plan to work on this let me know and I can reassign you

mds1 avatar Mar 09 '23 23:03 mds1

Hello, just adding my two 2ct here. We're building lot's of scripts and utilize logs assertions and even some ffi to give users a clear picture of what they are going to submit before actually doing it.

While this works fine with ppl using ledger (as the flow is stuck till they confirm) we noticed especially ppl using pk mess things up, as there's no confirmation needed. Even if they see sth is wrong it's usually to late (ofc, there's dry run etc, but that's not the point here).

Therefore i'm wondering if instead of having a vm.confirmContinue cheatcode it would make more sense to just have a --requireConfirm on the script command which would force you to enter before every tx broadcast?

I assume ppl either always or never want this, so using a cheatcode seems a bit overkill to me.

sakulstra avatar Jun 29 '23 19:06 sakulstra

Yea the title is outdated, the current UX that was agreed on is a flag as described in this comment: https://github.com/foundry-rs/foundry/issues/2399#issuecomment-1212485018

mds1 avatar Jun 29 '23 22:06 mds1

@mds1 should we close this considering that we now have vm.prompt* cheats?

klkvr avatar Apr 20 '24 20:04 klkvr