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

Use keystores in Scaffold-Eth #878

Open
wants to merge 33 commits into
base: foundry
Choose a base branch
from
Open

Use keystores in Scaffold-Eth #878

wants to merge 33 commits into from

Conversation

jrcarlos2000
Copy link
Collaborator

Description

Adds keystore for yarn generate and also for all scripts run inside script/deploy.s.sol.
keystore is saved to '~/.foundry/keystores/scaffold-eth-default'

yarn account:generate // generates a random wallet and saves to keystore
yarn account:import // uses prompt to ask for private key and password

Additional Information

Related Issues

@rin-st
Copy link
Collaborator

rin-st commented Jul 2, 2024

Hey @jrcarlos2000 ! Overall working great!

But I noticed some things we possibly need to change:

  • We need to add information to readme that after yarn:deploy we need to enter empty password when working locally or password we set for other chains
  • I think it's better to use another keystore for account:generate. Let's say I want to deploy my contract to sepolia. I run account:generate and then deploy it. Then I want to change contract, so I work locally again, and when yarn chain my generated sepolia account is gone with all my sepolia eth. So I need to save private key of sepolia account somewhere or generate a new one. But as I understand if we will use another keystore this problem disappears.
  • yarn account doesn't work
  • I'm getting this warning all the time on deploy, even when deploying to sepolia
Warning (9302): Return value of low-level calls not used.
  --> script/DeployHelpers.s.sol:33:9:
   |
33 |         deployer.call{ value: SCAFFOLD_BASE_BALANCE }("");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[⠆] Compiling...

Can we get rid of it?

  • yarn deploy:verify --network sepolia returns error
Submitting verification for [lib/forge-std/src/Base.sol:CommonBase] 0xB3763733d9C88b1C9Db929319a6763F761D1661E.
Encountered an error verifying this contract:
Response: `NOTOK`
Details: `Invalid constructor arguments provided. Please verify that they are in ABI-encoded format`

@jrcarlos2000
Copy link
Collaborator Author

jrcarlos2000 commented Jul 3, 2024

thanks @rin-st , yes still testing out what would be the best Dev Ex here before coming up with an updated README.

I think it's better to use another keystore for account:generate. Let's say I want to deploy my contract to sepolia. I run account:generate and then deploy it. Then I want to change contract, so I work locally again, and when yarn chain my generated sepolia account is gone with all my sepolia eth. So I need to save private key of sepolia account somewhere or generate a new one. But as I understand if we will use another keystore this problem disappears.

so for this, we can do a pre-check, only generates a wallet after yarn chain, when there is no existing keystore.

I'm getting this warning all the time on deploy, even when deploying to sepolia

yes, will get rid of this

yarn deploy:verify --network sepolia returns error

i think you need to foundryup

@jrcarlos2000
Copy link
Collaborator Author

jrcarlos2000 commented Jul 3, 2024

update : i have made a change:

when you do yarn account:generate or yarn account:import your keystore will be named scaffold-eth-custom, users will be able to set this on .env , please check the .env.example. so they can either used the anvil account ( scaffold-eth-default) or the imported / generated account ( at scaffold-eth-custom )

we also allow them to give it a name if provided.

yarn account:generate --name KEYSTORE_NAME , will need to update on .env

yarn account:import --name KEYSTORE_NAME , will need to update on .env

KEYSTORE_NAME defaults to scaffold-eth-custom

yarn account works and will use the provided keystore name on .env it will require you to unlock this account with password

@rin-st
Copy link
Collaborator

rin-st commented Jul 3, 2024

Thanks for the quick fixes! 🔥

Regarding yarn account. Could you take address from scaffold-eth-custom keystore, not as parameter? It worked without parameters before and also we don't need to save address before passing to yarn account

@jrcarlos2000
Copy link
Collaborator Author

Thanks for the quick fixes! 🔥

Regarding yarn account. Could you take address from scaffold-eth-custom keystore, not as parameter? It worked without parameters before and also we don't need to save address before passing to yarn account

yes this will be taken from the .env , it will be set to scaffold-eth-default first and scaffold-eth-custom when user has generated a wallet. However you still need to unlock it. we still need the password to decrypt the keystore.

cc: @technophile-04

@technophile-04
Copy link
Collaborator

technophile-04 commented Jul 5, 2024

After running yarn chain and then yarn deploy it gives:

image

Also after doing yarn account:generate and then yarn account (to view the generated account details) I get :
image

@technophile-04
Copy link
Collaborator

technophile-04 commented Jul 10, 2024

Tysm Carlos for this!!

As I understand ETH_KEYSTORE_ACCOUNT basically denotes which keystore to use.

SE-2 basically will be interacting with two keystore named:

  1. scaffold-eth-default => This keystore uses anvil's 0th account private key, password is set to empty string ''
  2. scaffold-eth-custom => This will be generated when you run yarn account:generate, users sets the password.

I kinda like how we are getting account of keystore from .env. This means if I have created an keystore account "my-account", I could just add it in .env#ETH_KEYSTORE_ACCOUNT=my-account and everything should work in context of my-account address as deployer / sending transaction and also yarn account will list details of my-account address.

^ So if above is true(it seems to be, on my testing) then this makes yarn account:import kind of redundant? If people want to use their preferred account with PK then they can just update .env#ETH_KEYSTORE_ACCOUNT with their account?

Difference b/w current SE-2 flow and keystore flow is:

  1. Whenever you run yarn deploy(on localhost) you need to hit "Enter"
  2. After you run yarn account:generate, you have to manually update .env#ETH_KEYSTORE_ACCOUNT with scaffold-eth-custom to use account generated by you to deploy contract.
  3. If you do yarn deploy(on localhost) after pt.2, user has to enter scaffold-eth-custom password instead of hitting enter since now we will be using scaffold-eth-custom account always.

Some DX improvements:

  1. Considering point 2. and 3, I think we should show the user whats the current account/address being used at that time when he is entering the password(Because I was kind of confused initally, should I hit "Enter" or use my generated account password without looking at .env)

  2. (optional): Umm thinking out loud, with keystore it feels like we should have single(testnet/mainnet) deploy account for all SE-2 instance. Currently(in this branch) it removes the previous instance scaffold-eth-custom from keystore and generates a new one.

    • Imagninary flow:
      1. Users run yarn generate => we check if scaffold-eth-custom is already present in keystore
      2. If not, we create scaffold-eth-cusom with random address.
      3. If its present, then in CLI we directly show the "Using address/account 0x23423423", and then "Enter the password"
      4. There could be an another script like yarn account:dangerous-re-generate (this will remove scaffold-eth-custom) and generate new one

But yeah would love to know others thought as well!

@jrcarlos2000
Copy link
Collaborator Author

Hey @technophile-04 thanks for the comments :

  1. the flow is correct
  2. the yarn account:import is a helper function for a non trivial operation
    cast wallet import ${1:-scaffold-eth-custom} --interactive // here is worth mentioning that the user can select the name if they pass --name {ACCOUNT_NAME} and use it later on the .env
    if i want to put my current pk into a keystore i would need to use this command anyways ( or something similar ) this is interactive which is good!
  3. what we can add is setting the custom or "named" account to be the one that .env points to, after the user calls yarn generate or yarn account:import . I wouldnt prefer this imo

packages/foundry/script/DeployHelpers.s.sol Outdated Show resolved Hide resolved
packages/foundry/script/DeployHelpers.s.sol Outdated Show resolved Hide resolved
packages/foundry/script/DeployHelpers.s.sol Outdated Show resolved Hide resolved
@jrcarlos2000
Copy link
Collaborator Author

Updates:

  • Updated from latest version of branch foundry
  • Added a --password flag, that uses a default password when on localhost , namely localhost, yarn chain && yarn account are also updated to follow this pattern.
  • Users will have to pass a password for other accounts they import
## Usage
yarn account --password PASSWORD ## defaults to localhost if not passed
yarn deploy --network NETWORK --password PASSWORD ## defaults to localhost if not passed
  • Unable to log the account being used, however if the decryption fails, it will log the corresponding account , users should be aware that the account they are using is the account they have already pointed to on their .env
Failed to decrypt keystore "/Users/carlosssramosss/.foundry/keystores/scaffold-eth-custom"
  • Added yarn:account:force-import to force override on scaffold-eth-custom account.

It looks cleaner right now @technophile-04

@escottalexander
Copy link
Contributor

escottalexander commented Aug 22, 2024

Testing this out and when I run yarn deploy I get this error:

[elliott@fedora foundry-deploy-test]$ yarn deploy
[⠢] Compiling...
No files changed, compilation skipped
error: the following required arguments were not provided:
  --keystore <PATHS>

Usage: forge script --keystore <PATHS> --fork-url <URL> --password <PASSWORDS> --broadcast --legacy --ffi --etherscan-api-key <KEY> <PATH> [ARGS]...

For more information, try '--help'.

Prior to running I ran yarn install, yarn chain and yarn account:generate. Did I miss a step?

@technophile-04
Copy link
Collaborator

Thanks @escottalexander for testing it out!

Could you please see if you have .env present in pacakges/foundry/.env and it should have this init ETH_KEYSTORE_ACCOUNT=scaffold-eth-default .

You don't need to create the .env it should have been automatically created when you first clone and yarn install if it didn't that it might be a bug which we need to solve (could you please tell the OS you are using?)

If .env was already present could you try upgrading foundry version. You could just run

foundryup

This should make all the foundry related stuff like anvil etc upto date and then could you try running yarn deploy again?

@escottalexander
Copy link
Contributor

escottalexander commented Aug 23, 2024

Thanks @technophile-04
Perhaps I did yarn install before being in the correct branch because I did have a .env but it did not have the ETH_KEYSTORE_ACCOUNT in it. Deleting the .env and performing a yarn install set up a .env with that variable and the deploy was successful. So hopefully just a user error on my part and not a real issue. Thanks!

@portdeveloper
Copy link
Contributor

portdeveloper commented Aug 25, 2024

EDIT: WAIT I WAS IN keystore-makefile branch! And the postinstall script is not there on that branch, hence the reason why we get that error! All is good!

Upon seeing @escottalexander 's reply here, I tested it with a clean install of SE-2 and I got the same error.

After yarn install, I tried to run yarn deploy. And below is what I get:
`
...
[⠊] Compiling...
[⠑] Compiling 31 files with Solc 0.8.26
[⠒] Solc 0.8.26 finished in 2.56s
Compiler run successful!
error: the following required arguments were not provided:
--keystore

Usage: forge script --keystore --fork-url --password --broadcast --legacy --ffi [ARGS]...

For more information, try '--help'.
make: *** [Makefile:16: deploy] Error 2
`
image

As @technophile-04 mentioned, yarn install should create a .env file in the directory shown above. (packages/foundry)

I tried to look around a bit but I couldn't figure out where the code that creates the .env file

Info about my system:

I run wsl2 ubuntu on a win11 EDU machine.

Linux version 5.15.153.1-microsoft-standard-WSL2 (root@941d701f84f1) (gcc (GCC) 11.2.0, GNU ld (GNU Binutils) 2.37) #1 SMP Fri Mar 29 23:14:13 UTC 2024

I have run foundryup to get foundry to the latest version.

@technophile-04
Copy link
Collaborator

Just merged the makefile PR, would love a last couple of tests.

git clone https://github.com/scaffold-eth/scaffold-eth-2.git foundry-se-2 --branch feat/keystore

Then

cd foundry-se-2 && yarn install

then try SE-2 magic command yarn chain, yarn deploy, yarn start they should work out of box hopefully.

Before deploying to testnet:

yarn account:generate

#enter some password

Then update .env#ETH_KEYSTORE_ACCOUNT=scaffold-eth-custom .

yarn account

# enter same password as above to unlock and see the balance

Send some eth to generate account on sepolia and then:

yarn deploy --network sepolia

# enter same password which you used early

This should deploy the contract to sepolia with generated account.

and lastly :

yarn verify --network sepolia

This should verify the contract

@escottalexander
Copy link
Contributor

escottalexander commented Aug 26, 2024

@technophile-04 yarn account:generate is successful but yarn account fails. I did update the .env to point to scaffold-eth-custom and I verified that the keystore exists in /.foundry/keystores. Let me know if you want me to try something to diagnose further.

[elliott@fedora foundry-se-2]$ yarn account:generate
Enter password: 
`scaffold-eth-custom` keystore was saved successfully. Address: 0x822bda2a647b1dba555d002964dba61c21d2b84c
Please update .env file with ETH_KEYSTORE_ACCOUNT=scaffold-eth-custom
[elliott@fedora foundry-se-2]$ yarn account
/bin/sh: line 1: source: .env: file not found
make[1]: *** [Makefile:30: verify-keystore] Error 1
🚫️ Unable to access keystore account scaffold-eth-custom

Please run `yarn account:generate` to generate deployer keystore account and update `ETH_KEYSTORE_ACCOUNT=scaffold-eth-custom` in `.env` file

@technophile-04
Copy link
Collaborator

Thanks @escottalexander for tests! Not sure what caused that problem but pushed a commit so that we don't do sourcing of .env in make file. Hopefully it should work now could you please try it once again?

@escottalexander
Copy link
Contributor

@technophile-04 I just repeated the steps in a fresh download of the repo (and removed my existing scaffold-eth-custom keystore) and this time it is asking for my password which is good but it fails after I give it my password. I tried twice (recreated the keystore) being very careful to enter my password correctly.

I am running Fedora Linux 40.

@technophile-04
Copy link
Collaborator

@technophile-04 I just repeated the steps in a fresh download of the repo (and removed my existing scaffold-eth-custom keystore) and this time it is asking for my password which is good but it fails after I give it my password. I tried twice (recreated the keystore) being very careful to enter my password correctly.

Strange, not sure whats happening did you get error as previous one after you enter password? Could you paste the error?

Also after you do yarn account:generate could you try just running plain command:

cast wallet address --account scaffold-eth-custom

# after entering the password it should log you wallet address

@escottalexander
Copy link
Contributor

escottalexander commented Aug 28, 2024

@technophile-04 This is the error I get:

[elliott@fedora foundry-se-2]$ yarn account
Enter keystore password:

🚫️ Unable to access keystore account scaffold-eth-custom

💡 If you haven't generated a deployer keystore account yet, please run `yarn account:generate`. Then update the `.env` file with `ETH_KEYSTORE_ACCOUNT=scaffold-eth-custom`

The simple cast command works. It prints the wallet address after I give it my password.

[elliott@fedora foundry-se-2]$ cast wallet address --account scaffold-eth-custom
Enter keystore password:
0x1e2a14D2038bD4DE7976c26Fa6724c81D89Eab20

I don't know much about C Makefiles but it appears when I run yarn account it tries to get my address by running cast wallet address(via make verify-keystore). This command seems to fail when I try it by itself:

[elliott@fedora foundry-se-2]$ cast wallet address
Error: 
Error accessing local wallet. Did you set a private key, mnemonic or keystore?
Run `cast send --help` or `forge create --help` and use the corresponding CLI
flag to set your key via:
--private-key, --mnemonic-path, --aws, --gcp, --interactive, --trezor or --ledger.
Alternatively, if you're using a local node with unlocked accounts,
use the --unlocked flag and either set the `ETH_FROM` environment variable to the address
of the unlocked account you want to use, or provide the --from flag with the address directly.

It works if I add the --account flag though.

[elliott@fedora foundry-se-2]$ cast wallet address --account scaffold-eth-custom
Enter keystore password:
0x1e2a14D2038bD4DE7976c26Fa6724c81D89Eab20

@technophile-04
Copy link
Collaborator

I don't know much about C Makefiles but it appears when I run yarn account it tries to get my address by running cast wallet address(via make verify-keystore). This command seems to fail when I try it by itself:

Ohh actually since we have .env configured in pacakges/foundry with ETH_KEYSTORE_ACCOUNT foundry picks that variable without us needing to explicitly mention scaffold-eth-custom it get gets that from .env :

image

Also was debugging this with @portdeveloper and I think we finally fixed the issue, seems like executing commands from makefile seems to pass argv at different position for different OS. But it should be good now and hopefully should work 🙌

@portdeveloper
Copy link
Contributor

I have just tested it all except the verify, will test it when etherscan is out of maintenance, and everything works great!
hyped up to see these changes in prod!!

@jrcarlos2000
Copy link
Collaborator Author

Have tested this, everything still works perfectly for me on Mac OS @technophile-04 😁

@carletex
Copy link
Member

This is working great!! I've been trying it with Shiv a little bit and pushed a couple of tweaks aa5e957

  • Don't require the password on localhost/default wallet (it's confusing if you run yarn account and require a password). We tried this before sourcing the env vars but it didn't work for some people. We went for a more naive approach. Could @portdeveloper & @escottalexander try now?
  • Added a generate alias (too ingrained in scaffold-eth haha)

Thanks!

@escottalexander
Copy link
Contributor

escottalexander commented Aug 30, 2024

@technophile-04 @carletex Works great! No issues this time. I was able to follow all the steps you posted including deploying to sepolia and verifying the contract. Nice work!

Also no issues when running the command on the default account. It did not prompt me for a password and everything ran correctly.

@portdeveloper
Copy link
Contributor

portdeveloper commented Sep 2, 2024

Tested it and it all works amazing!!!

Edit: Yes, everything is OK, I just checked with a brand-new contract and it works as intended.

@technophile-04
Copy link
Collaborator

technophile-04 commented Sep 2, 2024

Wanted to point this out to ask if this is the intended behavior or not.
Is it better to make it separate or keep it as is for ease of use?

Actually they are separate already, yarn deploy --network sepolia should only deploy the contract without verifying it.

I think maybe you felt this way because you got "contract already verified" when you deployed "YourContract.sol"? I think this is because etherscan is being smart marking contracts already verified which have same bytecode?

@portdeveloper
Copy link
Contributor

Wanted to point this out to ask if this is the intended behavior or not.
Is it better to make it separate or keep it as is for ease of use?

Actually they are separate already, yarn deploy --network sepolia should only deploy the contract without verifying it.

I think maybe you felt this way because you got "contract already verified" when you deployed "YourContract.sol"? I think this is because etherscan is being smart marking contracts already verified which have same bytecode?

That is right haha I should've tested it with a separate contract before adding that comment!

Now after testing with another contract, I saw that everything works as it should!
lesgoo

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.

None yet

6 participants