feat(`forge`): add createWallet() cheat code for new Wallet struct #5332

trevorgjohnson · 2023-07-07T16:40:29Z

Motivation

Solution

Add the struct Wallet to contain its privateKey, addr, publicKeyX, and publicKeyY.
createWallet was added to create Wallet's and sign, and getNonce were also added to as helpers for Wallet

trevorgjohnson · 2023-07-07T17:19:22Z

evm/src/executor/inspector/cheatcodes/env.rs

            },
        )??,
-        HEVMCalls::GetNonce(inner) => {
+        HEVMCalls::GetNonce1(inner) => {


Because of the placement of getNonce(Wallet) in abi/abi/HEVM.sol, getNonce(address) was renamed to HEVMCalls::GetNonce1

trevorgjohnson · 2023-07-07T17:20:19Z

evm/src/executor/inspector/cheatcodes/env.rs

+        HEVMCalls::GetNonce0(inner) => {
+            correct_sender_nonce(
+                b160_to_h160(data.env.tx.caller),
+                &mut data.journaled_state,
+                &mut data.db,
+                state,
+            )?;
+
+            // TODO:  this is probably not a good long-term solution since it might mess up the gas
+            // calculations
+            data.journaled_state.load_account(h160_to_b160(inner.0.addr), data.db)?;
+
+            // we can safely unwrap because `load_account` insert inner.0 to DB.
+            let account = data.journaled_state.state().get(&h160_to_b160(inner.0.addr)).unwrap();
+            abi::encode(&[Token::Uint(account.info.nonce.into())]).into()


Should all be the same as HEVMCalls::GetNonce1 except .get(&h160_to_b160(inner.0.addr))

trevorgjohnson · 2023-07-07T17:22:54Z

evm/src/executor/inspector/cheatcodes/util.rs

+    let pub_key = key.verifying_key().as_affine().to_encoded_point(false);
+    let pub_key_x = pub_key.x().ok_or("No x coordinate was found")?;
+    let pub_key_y = pub_key.y().ok_or("No y coordinate was found")?;
+
+    let pub_key_x = U256::from(pub_key_x.as_slice());
+    let pub_key_y = U256::from(pub_key_y.as_slice());


This is the best way I could find to extract the x and y coordinates. I'm not extremely familiar with the details of ECDSA algorithms, so I'm not 100% sure if this is accurate either.

trevorgjohnson · 2023-07-07T17:24:56Z

evm/src/executor/inspector/cheatcodes/util.rs

+    if let Some(label) = label {
+        state.labels.insert(addr, label);
+    }
+
+    Ok((addr, pub_key_x, pub_key_y, private_key).encode().into())


Ideally, private_key or addr should be reconstructed with pub_key_x and pub_key_y to ensure that the coordinates are correct, but I couldn't find a great way do that yet. I may make another commit adding that and reference that commit here, but I'm currently looking for suggestions 👍

I think it's ok right now to construct the addr from the private key—is the usual way you see it implemented in most libraries, even if it should be possible to construct it from the pubkeys—constructing from the privkey is just one more step.

trevorgjohnson · 2023-07-07T17:31:45Z

evm/src/executor/inspector/cheatcodes/util.rs

+        HEVMCalls::Sign0(inner) => sign(inner.0, inner.1.into(), data.env.cfg.chain_id.into()),
+        HEVMCalls::CreateWallet0(inner) => {
+            create_wallet(U256::from(keccak256(&inner.0)), Some(inner.0.clone()), state)
+        }
+        HEVMCalls::CreateWallet1(inner) => create_wallet(inner.0, None, state),
+        HEVMCalls::CreateWallet2(inner) => create_wallet(inner.0, Some(inner.1.clone()), state),
+        HEVMCalls::Sign1(inner) => {
+            sign(inner.0.private_key, inner.1.into(), data.env.cfg.chain_id.into())
+        }


Just wanted to compile a quick list to explain the functionality of each changed line here:

Sign0: the same as the old Sign. Just needed to be renamed for new function sign(Wallet)

CreateWallet0: function createWallet(string) to derive private key and label wallet with string

CreateWallet1: function createWallet(uint256) create new wallet with private key uint256 (no label)

CreateWallet2: function createWallet(uint256,string) create new wallet with private key uint256 and label with string

Sign1: function sign(Wallet,bytes32) but uses wallet's private key

We can add these as comments, they're great!

trevorgjohnson · 2023-07-07T17:33:15Z

testdata/cheats/Wallet.t.sol

+    function testCreateWalletStringPrivAndLabel() public {
+        bytes memory privKey = "this is a priv key";
+        Cheats.Wallet memory wallet = cheats.createWallet(string(privKey));
+
+        address expectedAddr = cheats.addr(wallet.privateKey);
+        assertEq(expectedAddr, wallet.addr);
+
+        string memory label = cheats.getLabel(wallet.addr);
+        assertEq(label, string(privKey), "labelled address != wallet.addr");
+    }


Ideally, there'd be another section of this test that checks the validity of wallet.publicKeyX and wallet.publicKeyY, but I wasn't sure what the best way of doing that was

One way would be to verify that the address derived from the coordinates equals the address derived via the addr(unit privKey)(address) cheatcode.

Deriving the Ethereum address is specified in the Yellow Paper "Appendix F: Signing Transactions" §134. A possible Solidity implementation is:

function addressOf(uint xCoordinate, uint yCoordinate) internal pure returns (address) { return address(uint160(keccak256(abi.encode(xCoordinate, yCoordinate)))); }

mds1 · 2023-07-07T23:03:55Z

@pmerkleplant what was your original use case for pubkeys? Mainly asking because there's three ways we can represent them, and I'm wondering if we should choose one and you can convert between them as needed (maybe tricky in solidity), or just give all 3 in the Wallet struct (but that feels cluttered)

// Approach 1
uint256 publicKeyX;
uint256 publicKeyY;

// Approach 2
bytes publicKey; // 0x04{x}{y}

// Approach 3;
bytes compressedPublicKey; // 0x02{x} or 0x03{x}

pmerkleplant · 2023-07-08T12:38:01Z

@pmerkleplant what was your original use case for pubkeys?

Our use case is to aggregate public keys, i.e. perform elliptic curve point addition. We also have to derive the Ethereum address of the resulting public key.

We hope that the cheatcode makes our fuzzing test suite faster, in which we receive sets of random private keys and compute and aggregate their public keys.

Therefore, the most suitable approach for us would be the first/currently implemented one, i.e. receiving the x and y Affine coordinates as uints.

I would personally argue against approach 3 as the y coordinate would need to be computed again, making even deriving the Ethereum address computation heavy and error-prone.

@mds1 Out of curiosity, could you elaborate on where approach 2 is coming from? Is this a format standardized somewhere?

@trevorgjohnson Many thanks for the work so far!

mds1 · 2023-07-08T17:24:05Z

Therefore, the most suitable approach for us would be the first/currently implemented one, i.e. receiving the x and y Affine coordinates as uints.
I would personally argue against approach 3 as the y coordinate would need to be computed again, making even deriving the Ethereum address computation heavy and error-prone.

Got it, thanks. I have a use case as part of ERC-5564/ERC-6538 where we may store compressed public keys in a registry, but starting with the x/y coordinates and compressing/uncompressing in solidity shouldn't be too bad, so I'd be ok with moving forward with the current approach for the Wallet struct

Out of curiosity, could you elaborate on where approach 2 is coming from? Is this a format standardized somewhere?

This is a convention from Standards for Efficient Cryptography Group where:

uncompressed pubkeys are 65 bytes (0x04 prefix + 32 byte x-coordinate + 32 byte y-coordinate
compressed pubkeys are 33 btyes (0x02 OR 0x03 prefix + 32 byte x-coordinate, where 0x02 means the y value is even, and 0x03 means it's odd

It's used by popular libs such as ethers.js and noble-secp256k1. You can read more about it and find references to more data in mastering ethereum here

…Nonce' to 'GetNonce1'

trevorgjohnson · 2023-07-10T13:46:23Z

I rebased the current version of `master

pmerkleplant

Approving the Solidity part for my use case. Note that I'm not skilled to review the Rust changes.

pmerkleplant · 2023-07-10T15:50:40Z

testdata/cheats/Wallet.t.sol

+        assertEq(label, string(privKey), "labelled address != wallet.addr");
+    }
+
+    function testCreateWalletPrivKeyNoLabel(uint248 pk) public {


Instead of receiving the private key as type uint248, one could receive a private key seed of type uint256 and create the private key via:

uint internal constant Q = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141; // Let pk ∊ [1, Q). uint pk = bound(pkSeed, 1, Q-1);

This would ensure the whole eligible set of private keys is testable. (Note that type(uint248).max < Q, meaning the highest possible private keys are not testable)

+1, this would be a stronger test (applies to the other tests also)

Agreed 👍 testing using a higher range of testable private keys was added here 3f7b4e8

thanks! just a nit, but you can remove the vm.assume(pkSeed != 0); checks now, since they are redundant given the bound usage

Ha yea that's a good catch! There was an issue with bound not being defined, so I fixed that and this nit in this commit here: 8c36615

trevorgjohnson · 2023-07-24T15:05:14Z

Hello @Evalir @mattsse, just wanted to check and see if you guys have had a chance to check out the newest changes to this PR yet. Thanks!

Austinhs · 2023-08-09T21:08:59Z

Please add :)

Evalir · 2023-08-09T21:13:58Z

ah whoops, we let this go stale but i think there's still interest here. @trevorgjohnson sorry! mind fixing the conflict to give this a review?

trevorgjohnson · 2023-08-09T21:56:35Z

ah whoops, we let this go stale but i think there's still interest here. @trevorgjohnson sorry! mind fixing the conflict to give this a review?

No worries! The conflicts should be resolved in d7bcc7d

Evalir

lgtm, fairly straightforward even if it's sensitive code.

my only ask is that let's comment out everything you laid out in the pr on the actual code, as they're great!

Evalir · 2023-08-10T15:08:06Z

evm/src/executor/inspector/cheatcodes/env.rs

+            // TODO:  this is probably not a good long-term solution since it might mess up the gas
+            // calculations
+            data.journaled_state.load_account(h160_to_b160(inner.0.addr), data.db)?;


@mattsse wdyt here? seems ok, but might mess with gas yes

Evalir · 2023-08-10T15:08:38Z

evm/src/executor/inspector/cheatcodes/util.rs

    Ok((sig.v, r_bytes, s_bytes).encode().into())
 }

+fn create_wallet(private_key: U256, label: Option<String>, state: &mut Cheatcodes) -> Result {


let's add a bit of docs on what it does, even if it's a bit obvious :D

Sounds good to me! Were you thinking about just adding some comments above this? Or just adding some create_wallet documentation in the foundry book?

Actually, I decided to add them here since I saw that multiple other functions also included some documentation. Let me know what you think 👍

Evalir · 2023-08-10T15:10:18Z

evm/src/executor/inspector/cheatcodes/util.rs

+    if let Some(label) = label {
+        state.labels.insert(addr, label);
+    }
+
+    Ok((addr, pub_key_x, pub_key_y, private_key).encode().into())


I think it's ok right now to construct the addr from the private key—is the usual way you see it implemented in most libraries, even if it should be possible to construct it from the pubkeys—constructing from the privkey is just one more step.

Evalir · 2023-08-10T15:11:39Z

evm/src/executor/inspector/cheatcodes/util.rs

+        HEVMCalls::Sign0(inner) => sign(inner.0, inner.1.into(), data.env.cfg.chain_id.into()),
+        HEVMCalls::CreateWallet0(inner) => {
+            create_wallet(U256::from(keccak256(&inner.0)), Some(inner.0.clone()), state)
+        }
+        HEVMCalls::CreateWallet1(inner) => create_wallet(inner.0, None, state),
+        HEVMCalls::CreateWallet2(inner) => create_wallet(inner.0, Some(inner.1.clone()), state),
+        HEVMCalls::Sign1(inner) => {
+            sign(inner.0.private_key, inner.1.into(), data.env.cfg.chain_id.into())
+        }


We can add these as comments, they're great!

trevorgjohnson · 2023-08-11T12:54:37Z

evm/src/executor/inspector/cheatcodes/util.rs

+        // [function sign(uint256,bytes32)] Used to sign bytes32 digests using the given private key
+        HEVMCalls::Sign0(inner) => sign(inner.0, inner.1.into(), data.env.cfg.chain_id.into()),
+        // [function createWallet(string)] Used to derive private key and label the wallet with the
+        // same string
+        HEVMCalls::CreateWallet0(inner) => {
+            create_wallet(U256::from(keccak256(&inner.0)), Some(inner.0.clone()), state)
+        }
+        // [function createWallet(uint256)] creates a new wallet with the given private key
+        HEVMCalls::CreateWallet1(inner) => create_wallet(inner.0, None, state),


@Evalir, I looked around for an existing doc schema for each of these match call arms, but I couldn't find any so I made my own. Let me know what you think 👍

Evalir · 2023-08-15T14:59:05Z

Amazing! just need forge-std PR + book update

trevorgjohnson commented Jul 7, 2023

View reviewed changes

Evalir requested review from Evalir and mattsse July 7, 2023 18:41

trevorgjohnson added 8 commits July 10, 2023 08:44

chore: add Wallet struct and other relevant functions to 'HEVM.sol'

7079022

feat: add 'HEVMCalls::GetNonce0' to env utils for Wallet, rename 'Get…

66b8bf5

…Nonce' to 'GetNonce1'

feat: add CreateWallet/Sign to HEVM match in cheat utils

e8aeeb6

chore: add Wallet struct and other relevant functions to 'Cheats.sol'

81957b8

test: add tests for 'createWallet' in Wallet.t.sol

d837d99

test: add tests for getNonce/sign in Wallet.t.sol

6e3fe1d

chore: remove 'unwrap' after 'create_wallet' calls

1762683

style: forge fmt 'testdata/cheats/Wallet.t.sol'

0dcb4b2

trevorgjohnson force-pushed the feat/pubk-coords branch from 0abac5b to 2512ac8 Compare July 10, 2023 13:44

test: add x & y to addr check to Wallet.t.sol

ee61825

trevorgjohnson force-pushed the feat/pubk-coords branch from 2512ac8 to ee61825 Compare July 10, 2023 13:52

chore: rename 'cheats' to 'vm' in Wallet.t.sol

a8a423a

trevorgjohnson requested a review from pmerkleplant July 10, 2023 15:34

pmerkleplant approved these changes Jul 10, 2023

View reviewed changes

Merge branch 'master' into feat/pubk-coords

d7bcc7d

test: change pkSeed to uint256 to check whole private key range

3f7b4e8

Evalir approved these changes Aug 10, 2023

View reviewed changes

Add 'bound' func, remove 'vm.assume(pk != 0)'

f4e105d

trevorgjohnson force-pushed the feat/pubk-coords branch from 8c36615 to f4e105d Compare August 10, 2023 20:51

docs: add doc comments surrounding 'create_wallet'

7cc1d36

trevorgjohnson commented Aug 11, 2023

View reviewed changes

Merge branch 'master' into feat/pubk-coords

cf67cc5

Evalir approved these changes Aug 15, 2023

View reviewed changes

Evalir merged commit 410c817 into foundry-rs:master Aug 15, 2023

Evalir mentioned this pull request Aug 15, 2023

feat(forge): Cheatcode to receive public key of private key #4790

Closed

trevorgjohnson deleted the feat/pubk-coords branch August 15, 2023 15:08

This was referenced Aug 15, 2023

chore(vm): add createWallet() for Wallet struct foundry-rs/forge-std#435

Merged

docs(cheatcodes): add createWallet doc page foundry-rs/book#986

Merged

feat(forge): add createWallet() cheat code for new Wallet struct #5332

feat(forge): add createWallet() cheat code for new Wallet struct #5332

Uh oh!

Conversation

trevorgjohnson commented Jul 7, 2023

Motivation

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmerkleplant Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 commented Jul 7, 2023

Uh oh!

pmerkleplant commented Jul 8, 2023

Uh oh!

mds1 commented Jul 8, 2023

Uh oh!

trevorgjohnson commented Jul 10, 2023

Uh oh!

pmerkleplant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevorgjohnson Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevorgjohnson commented Jul 24, 2023

Uh oh!

Austinhs commented Aug 9, 2023

Uh oh!

Evalir commented Aug 9, 2023

Uh oh!

trevorgjohnson commented Aug 9, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Evalir commented Aug 15, 2023

Uh oh!

feat(`forge`): add createWallet() cheat code for new Wallet struct #5332

feat(`forge`): add createWallet() cheat code for new Wallet struct #5332

pmerkleplant Jul 8, 2023 •

edited

Loading

mds1 Aug 9, 2023 •

edited

Loading

trevorgjohnson Aug 10, 2023 •

edited

Loading