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(forge): add derive cheatcode for mnemonics #2299

Merged
merged 5 commits into from Jul 13, 2022

Conversation

devanoneth
Copy link
Contributor

Motivation

I want to live in a world where we can run forge script MyScript and not need to worry about sourcing environment variables or adding any flags, rather allowing the script to setup everything it needs in the script itself. I think this is more declarative and leads to better scripts.

Solution

Add a derive cheatcode which allows to derive private keys from a given mnemonic string or file path. I am adding this PR early, on its own it does not seem to do much, but I would also like to add a remember cheatcode which will add private keys to the current forge invocation so that you can later broadcast with them.

It would allow for scripts such as the following:

Solenv.config();

uint256 deployerPrivateKey = vm.derive(vm.envString("MNEMONIC"), 0);
address deployer = vm.remember(deployerPrivateKey);

uint256 governancePrivateKey = vm.derive(vm.envString("MNEMONIC"), 1);
address governance = vm.remember(governancePrivateKey);

vm.startBroadcast(deployer);
Contract contract = new Contract(governance);
vm.stopBroadcast();

vm.startBroadcast(governance);
contract.addPair(vm.envString("PAIR"));
vm.stopBroadcast();

So, before I add the remember cheatcode I'd like to get feedback on this early. :) Thanks!

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.

great, ty!

lgtm, just a suggestion re naming

Comment on lines 42 to 43
derive(string,uint32)(uint256)
derive(string,string,uint32)(uint256)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to be a bit more explicit here about the naming,

wdyt of deriveKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that works for me, just updated in the my last commit

@mattsse
Copy link
Member

mattsse commented Jul 13, 2022

but I would also like to add a remember cheatcode which will add private keys to the current forge invocation

this would be possible, there is now a Context container type in Cheatcodes that's intended for this, for example that's where we currently store file handles for the file related cheatcodes

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.

lgtm, needs lint

any objections @gakonst @onbjerg ?

it'd be great if you could add a reference entry to the book as well

@devanoneth
Copy link
Contributor Author

Oops, my bad on missing the last lint! Yes, re the book I plan to get the remember cheatcode in too ASAP and then will add both of them to the book.

@@ -71,6 +74,21 @@ fn sign(private_key: U256, digest: H256, chain_id: U256) -> Result<Bytes, Bytes>
Ok((sig.v, r_bytes, s_bytes).encode().into())
}

fn derive_key(mnemonic: &str, path: &str, index: u32) -> Result<Bytes, Bytes> {
let derivation_path = format!("{}{}", path, index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd rather not assume that the path ends in a /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally wanted to do that, but changed it to fit in line more with ethers-rs.

With that said, I actually think I made it worse lol.

I'd be fine to add in the / for the path so that the user is not expected to. We could also hold the users hand and check for it and only add it if it's not there.

Copy link
Collaborator

@joshieDo joshieDo Jul 13, 2022

Choose a reason for hiding this comment

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

In that case, the path is a const and not a user input though...

We could also hold the users hand and check for it and only add it if it's not there.

I like it

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

love the principle of making scripts as declarative as possible.

merging so we close the feedback loop and 👍 on trying to make the path derivation more robust, doubt people will add too many custom paths though

@gakonst gakonst merged commit 6d30b62 into foundry-rs:master Jul 13, 2022
mattsse pushed a commit to mattsse/foundry that referenced this pull request Jul 14, 2022
* implement derive cheatcode for mnemonic derivation in scripts

* fix formatting

* remove unnecessary semi-colons

* change derive function to deriveKey

* lint fix
@devanoneth devanoneth deleted the feat/derive-cheatcode branch August 31, 2022 21:18
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

4 participants