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: allow supplying function name via forge script --sig #7518

Merged
merged 4 commits into from
Mar 30, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Mar 29, 2024

Motivation

Having script function with many arguments can be annoying as you have to write stuff like forge script --sig run(address,uint256,(address,uint256)...

With this PR if only function name is provided as --sig argument and there is exactly one function with such name in the script ABI, then it will be used. Example above becomes just forge script --sig 'run' 'ARG 1' 'ARG 2'

cc @mds1 I remember we had discussion related to script UX and you mentioned that such UX improvements might result in footguns, so curious to know what you think about this

@klkvr klkvr requested review from DaniPopes and mattsse as code owners March 29, 2024 13:29
@mds1
Copy link
Collaborator

mds1 commented Mar 29, 2024

cc @mds1 I remember we had discussion related to script UX and you mentioned that such UX improvements might result in footguns, so curious to know what you think about this

Hmm any more context here to jog my memory? From an initial read of the PR description this sounds like a nice UX improvement that should be safe. I guess one potential footgun—which seems to be mitigated based on your description—is if you have:

function run(uint x) public {
  run(x, defaultY);
}

function run(uint x, uint y) public {
 // --- snip ---
}

and execute forge script --sig 'run' 'ARG 1' and just forgot to put arg2 and now used the wrong default arg2 value. Whereas previously you need mismatched sig + arg count. But it sounds like this PR prohibits this failure mode anyway if both methods are named run which is ok with me. I would also be ok with inferring/resolving overloads in that case (may not always be possible though, e.g. when methods are run(uint, uint) and run(uint, int))

But otherwise this lgtm and I might need to think more to remember another footgun we discussed

@klkvr
Copy link
Member Author

klkvr commented Mar 29, 2024

Hmm any more context here to jog my memory?

I believe it was this one: #5282

forge script --sig 'run' 'ARG 1' and just forgot to put arg2 and now used the wrong default arg2 value.

yeah, that's why we reject duplicates instead of potentialy trying to match by arguments count

I would also be ok with inferring/resolving overloads in that case (may not always be possible though)

I think this PR will encourage users to not have such overloads in scripts, as with this this change you can avoid typing function parameters at all by just having unique names, so no need to have logic for overloads handling imo

@mds1
Copy link
Collaborator

mds1 commented Mar 29, 2024

Thanks! Ok, I'm onboard with the behavior suggested in this PR and not supporting overloads

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.

that's a pretty neat UX improvement.

could we test the failure case for overloaded functions in a test?

@klkvr klkvr requested a review from Evalir as a code owner March 30, 2024 15:34
@klkvr klkvr requested a review from mattsse March 30, 2024 15:34
@mattsse mattsse merged commit d94e3c6 into master Mar 30, 2024
19 checks passed
@mattsse mattsse deleted the klkvr/script-sig-fn-name branch March 30, 2024 19:33
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