Rewrite deploy script to foundry #491
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
scripts/deploy.py
Outdated
| # Add keystore password when environment variable was set, e.g. for non-interactive mode | ||
| ETH_KEYSTORE_PASSWORD = os.environ.get('ETH_KEYSTORE_PASSWORD') | ||
| if ETH_KEYSTORE_PASSWORD: | ||
| deploy_cmd.extend(["--password", ETH_KEYSTORE_PASSWORD]) |
There was a problem hiding this comment.
I don't think this is a good practice at all.
Putting secrets into the environment is the best way to have them leaked.
We should default to prompting the user for a password always.
The crafter only needs to do that once, so it's not a huge burden.
There was a problem hiding this comment.
As the comment suggests, this has only been done for supporting non-interactive mode (i.e. CI which we use for deploying spells).
Developers should not set this env normally, and this is mentioned in the README:
Line 39 in 82b40ae
In the normal execution password will be prompted.
There was a problem hiding this comment.
But why do we need non-interactive mode for spells?
Actually, there's a point to be made that non-interactive mode should be discouraged nowadays.
This was largely used back when using bash scripts and Dapp Tools to deploy multiple contracts, because there wasn't a concept of "session", so every command was run as if it was standalone.
With Foundry scripts we have a different scenario. When performing a multi-contract deployment using a script, the password is prompted only once, so the benefit of non-interactive mode is way smaller.
There was a problem hiding this comment.
I found a better way for supporting this without having to accept a plaintext password.
Turns out Foundry already supports this via:
--password-file <PASSWORD_FILE>
The keystore password file path.
Used with --keystore.
[env: ETH_PASSWORD=]
I therefore removed the extra variable and its mentions.
There was a problem hiding this comment.
Welp, having an unencrypted file with a password in your computer is probably as bad as having in it as an env var.
Most runtimes out there allows the program to read from any directory/file the user running it has read access to.
Anyway, as long as we don't encourage this practice in this repo, I'm good with it.
scripts/deploy.py
Outdated
| print('Re-running the tests...') | ||
| test_logs = subprocess.run([ | ||
| 'make', 'test', | ||
| f'block="{tx_block}"', |
There was a problem hiding this comment.
I'm not sure if we should fix the block here.
Reviewers are instructed to run this with no block=N argument, and CI also runs it against the latest block.
scripts/deploy.py
Outdated
| CHAIN_ID = '1' | ||
| PATH_TO_SPELL = 'src/DssSpell.sol' | ||
| SPELL_CONTRACT_NAME = 'DssSpell' | ||
| PATH_TO_CONFIG = 'src/test/config.sol' |
There was a problem hiding this comment.
Nit: let's not introduce visual alignment.
In Python code, whitespaces are syntactic tokens. Even though it's fine in this specific instance. Here's a quick comparison:
I'd suggest using some well-known formatter for Python like black, autopep8 or yapf:
| CHAIN_ID = '1' | |
| PATH_TO_SPELL = 'src/DssSpell.sol' | |
| SPELL_CONTRACT_NAME = 'DssSpell' | |
| PATH_TO_CONFIG = 'src/test/config.sol' | |
| CHAIN_ID = '1' | |
| PATH_TO_SPELL = 'src/DssSpell.sol' | |
| SPELL_CONTRACT_NAME = 'DssSpell' | |
| PATH_TO_CONFIG = 'src/test/config.sol' |
There was a problem hiding this comment.
I've run pylint previously to detect any code problems. I've also ran black now to format the code
| # Verify the contract | ||
| subprocess.run([ | ||
| 'make', 'verify', | ||
| f'addr={spell_address}', | ||
| ], check=True) | ||
|
|
||
| # Re-run the tests | ||
| print('Re-running the tests...') | ||
| test_logs = subprocess.run(['make', 'test'], capture_output=True, text=True, check=False) | ||
| print(test_logs.stdout) | ||
|
|
||
| if test_logs.returncode != 0: | ||
| print(test_logs.stderr) | ||
| print('Ensure Tests PASS before commiting the `config.sol` changes!') | ||
| sys.exit(test_logs.returncode) |
There was a problem hiding this comment.
Nit: probably should invert the steps here. If the test fails, it shouldn't verify the contract.
There was a problem hiding this comment.
The current checklist explicitly mentions that spell should be first verified and then tested locally: https://github.com/sky-ecosystem/pe-checklists/blob/161895d52e144fec1e4d8cca52f1379ce63950cb/spell/spell-crafter-mainnet-workflow.md?plain=1#L213-L214
I don't have a strong preference, but I'd keep the current process
This PR rewrites deployment script to use Foundry instead of dapp.tools. It is the continuation of #448 from the fresh
master, given that verification has already been revamped in #469The easiest way of testing this PR is to deploy a spell to anvil/tenderly fork, commenting out verification command. Note that deploying on a local fork only allows to check code verification against local Sourcify node, i.e. not using
make verify. It is not an actual problem, since verification code itself hasn't been modified in this PR.See the PR inside the checklists repo: sky-ecosystem/pe-checklists#51