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: add inputs argument #12

Merged
merged 6 commits into from
Nov 11, 2023
Merged

Conversation

ghost
Copy link

@ghost ghost commented Nov 9, 2023

This may help @nlordell.

Originally I thought params would be a good arg, but it wouldn't accept it due to prefix.

@ghost
Copy link
Author

ghost commented Nov 9, 2023

If you're happy with approach, I can add updated documentation.

cli/src/main.rs Outdated
Comment on lines 58 to 59
#[arg(short, long)]
inputs: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

I liked your suggestion of calling this params to print out the createProxyWithNonce parameters.

The issue with it not working with the existing prefix flag is that #[arg(short)] defaults to using the first letter of there field name. You can fix that by specifying something else, for example:

Suggested change
#[arg(short, long)]
inputs: bool,
#[arg(short = 'P', long)]
params: bool,

This way, -P is used to print parameters, and -p is used for setting a prefix.

@nlordell
Copy link
Owner

Changes and approach looks good! Thanks for taking care of this :) Just a comment around the parameter name.

@ghost
Copy link
Author

ghost commented Nov 10, 2023

Hey @nlordell

Changed as advised.

Updated the docs, included a note about Etherscan.

There are two more commits:

  1. To me it makes sense for creating the safe to be above Unsupported Chains, as it's the more common immediate usage
  2. I suggest adding a note about safe 1.3.0. Currently it seems safe-service doesn't automatically pick up on the creation event from 1.4.1 safes - unless I'm understanding incorrectly, so it seems like a good idea to guide people to 1.3.0 vanity generation?

I realise now these should probably be different pull requests. Happy either way as this is your repo :)


Fill the fields in function `3. createProxyWithNonce` using the generated outputs.


Copy link
Owner

Choose a reason for hiding this comment

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

nit: extra newline

Suggested change

@nlordell
Copy link
Owner

To me it makes sense for creating the safe to be above Unsupported Chains, as it's the more common immediate usage

100% agree.

Currently it seems safe-service doesn't automatically pick up on the creation event from 1.4.1 safes

Oh no! Then yes, you should probably roll back to using v1.3.0 Safes for now. I also just double checked ant the wallet interface creates a v1.3.0 Safe as well, so it looks like v1.4.1 is not super well supported yet. Thanks for bringing this up!

@nlordell nlordell merged commit 5faa6eb into nlordell:main Nov 11, 2023
1 of 2 checks passed
@nlordell
Copy link
Owner

@kennedybaird - I rolled back the CLI to use Safe v1.3.0 because of compatibility concerns with the Safe infrastructure and Wallet interface. Thanks a bunch for bringing this up!

@ghost
Copy link
Author

ghost commented Nov 11, 2023 via email

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.

2 participants