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

Issue with arguments using forge create #473

Closed
crisgarner opened this issue Jan 16, 2022 · 9 comments
Closed

Issue with arguments using forge create #473

crisgarner opened this issue Jan 16, 2022 · 9 comments
Labels
C-forge Command: forge Cmd-forge-create Command: forge create D-easy Difficulty: easy good first issue Good for newcomers P-low Priority: low T-bug Type: bug

Comments

@crisgarner
Copy link
Contributor

crisgarner commented Jan 16, 2022

Currently deploying a contract with multiple arguments require forge create command to use multiple times the param --constructor-args

forge create Token --constructor-args "My token" --constructor-args "TKN" --constructor-args 18 --mnemonic-path XXX --rpc-url XXX

Suggestion, change it to only require one argument call

forge create Token --constructor-args "My token" "TKN" 18 --mnemonic-path XXX --rpc-url XXX

Fix might be around here: https://github.com/gakonst/foundry/blob/master/cli/src/cmd/create.rs#L54-L55

@gakonst
Copy link
Member

gakonst commented Jan 16, 2022

@gakonst
Copy link
Member

gakonst commented Jan 17, 2022

Should we deprecate forge create altogether in favor of the whole forge deploy workflow, or would it still be useful for "quick" deployments of single files? #402

@gakonst gakonst added T-bug Type: bug C-forge Command: forge labels Jan 17, 2022
@brockelmore
Copy link
Member

i think it will remain useful - create is for solo contracts, deploy is deployment management of multiple imo

@fmhall
Copy link

fmhall commented Jan 17, 2022

Can we add a --verify flag to forge create? Does that make sense?

@gakonst
Copy link
Member

gakonst commented Jan 17, 2022

@fmhall this is tracked in #287 #354

@onbjerg
Copy link
Member

onbjerg commented Jan 18, 2022

The behavior we want is Arg::multiple_values(true) (see docs) but it is not available in their derive crate, possibly because they deem it to be unsafe? What we have now is Arg::multiple_occurences(true).

We can either do Arg::mutiple_values(true) (despite the warnings) or we could add the parameters as direct arguments on the CLI (i.e. forge create MyContract "first arg" "second arg" 2 3 4)

@gakonst
Copy link
Member

gakonst commented Jan 18, 2022

Hmm reading through the warnings, it seems like it should be fine? I don't think we'd ever have any positional arguments for --constructor-args, only named args? We can enable this with raw in clap derive if I'm not mistaken - I'd be supportive of doing that and adding a note / link to when it could be dangerous in a doc.

@onbjerg
Copy link
Member

onbjerg commented Jan 18, 2022

Yeah I think in this case it is fine

@onbjerg onbjerg added Cmd-forge-create Command: forge create D-easy Difficulty: easy good first issue Good for newcomers P-low Priority: low labels Jan 28, 2022
@calldata
Copy link
Contributor

Anyone take this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-create Command: forge create D-easy Difficulty: easy good first issue Good for newcomers P-low Priority: low T-bug Type: bug
Projects
None yet
Development

No branches or pull requests

6 participants