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.

Sign up for GitHub

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

Jump to bottom

feat(evm): support multiple languages for deriveKey cheatcode #4688

Merged
merged 7 commits into from
Jun 23, 2023
Merged

feat(evm): support multiple languages for deriveKey cheatcode #4688

merged 7 commits into from
Jun 23, 2023

Conversation

0xYYY
Copy link
Contributor

@0xYYY 0xYYY commented Apr 2, 2023

Motivation

Resolves #4560.

Solution

Add support for other languages' wordlist in summa-tx/coins#119.

@0xYYY 0xYYY mentioned this pull request Apr 2, 2023
Merged
mattsse
mattsse requested changes Apr 2, 2023
View reviewed changes
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.

nice!

nit re str matching

evm/src/executor/inspector/cheatcodes/util.rs Outdated
}
HEVMCalls::DeriveKey1(inner) => derive_key(&inner.0, &inner.1, inner.2),
HEVMCalls::DeriveKey1(inner) => derive_key::<English>(&inner.0, &inner.1, inner.2),
HEVMCalls::DeriveKey2(inner) => match inner.2.as_str() {
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 have this as standalone function derive_key_with_wordlist that accepts the wordlist as str. perhaps even worth having an enum for this

@prestwich
Copy link
Contributor

coins-bip39@0.8.4 contains non-english wordlists. Thanks @0xYYY!

@0xYYY
Copy link
Contributor Author

0xYYY commented Apr 10, 2023

pending another fix summa-tx/coins#120

DaniPopes
DaniPopes reviewed Apr 10, 2023
View reviewed changes
Cargo.toml Outdated
ethers-signers = { git = "https://github.com/0xYYY/ethers-rs", branch = "chore/bump-bitcoin-rs" }
ethers-middleware = { git = "https://github.com/0xYYY/ethers-rs", branch = "chore/bump-bitcoin-rs" }
ethers-etherscan = { git = "https://github.com/0xYYY/ethers-rs", branch = "chore/bump-bitcoin-rs" }
ethers-solc = { git = "https://github.com/0xYYY/ethers-rs", branch = "chore/bump-bitcoin-rs" }
Copy link
Member

@DaniPopes DaniPopes Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We don't depend on ethers git anymore, can this be in a added in the next ethers release?

@0xYYY 0xYYY mentioned this pull request Apr 12, 2023
Merged
3 tasks
@Evalir
Copy link
Member

Evalir commented May 5, 2023

hey @0xYYY — do you plan on getting back to this soon?

@0xYYY
Copy link
Contributor Author

0xYYY commented May 6, 2023

hey @0xYYY — do you plan on getting back to this soon?

yeah, plan to finish this around next week

@0xYYY 0xYYY force-pushed the feat/evm-cheatcode-derive-multilang branch from 3b7a094 to d1d4f5e Compare May 8, 2023 16:01
@0xYYY
Copy link
Contributor Author

0xYYY commented May 8, 2023

added a standalone derive_key_with_wordlist function, bumped ethers version, rebased on master, all tests have passed, ready for review

cc @Evalir @mattsse

@0xYYY 0xYYY requested a review from mattsse May 8, 2023 16:29
@Evalir Evalir self-requested a review May 8, 2023 16:31
Evalir
Evalir approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

this lgtm—deferring to matt for final approval. we could maybe add a simple failing test to ensure it fails as expected?

@Evalir
Copy link
Member

Evalir commented May 10, 2023

hmm seems we have some tests failing?

@0xYYY
Copy link
Contributor Author

0xYYY commented May 10, 2023 •
edited
Loading

hmm seems we have some tests failing?

yeah, but weirdly can't see any details about why the tests failed

Screenshot 2023-05-10 at 23 01 38 Screenshot 2023-05-10 at 23 01 26

@0xYYY
Copy link
Contributor Author

0xYYY commented May 10, 2023

hmm seems we have some tests failing?

seems like the failing tests have ben re-ran and all tests have passed now

@Evalir
Copy link
Member

Evalir commented May 10, 2023 •
edited
Loading

yup guessed they were just outage/unrelated failures, nice! will wait for matt to review/merge—nice work :)

@Evalir
Copy link
Member

Evalir commented May 17, 2023

hey @0xYYY — do you think we could rebase this? sorry for the extra work, but we recently refactored the abigen part of cheatcodes so will need to redo that bit

@0xYYY
Copy link
Contributor Author

0xYYY commented May 19, 2023

hey @0xYYY — do you think we could rebase this? sorry for the extra work, but we recently refactored the abigen part of cheatcodes so will need to redo that bit

no prob, on it

@0xYYY 0xYYY force-pushed the feat/evm-cheatcode-derive-multilang branch from 06b5467 to 16a86e6 Compare May 21, 2023 10:11
Evalir
Evalir approved these changes May 21, 2023
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

aight this seems ok now—just seems we have an unrelated ci failure due to a flaky test

@0xYYY
Copy link
Contributor Author

0xYYY commented May 23, 2023

will wait for matt to review/merge—nice work :)

hi @mattsse, pls let me know if there are any changes that i can make to push forward this pr, thx

@0xYYY 0xYYY requested a review from DaniPopes May 23, 2023 15:29
Evalir
Evalir approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

This is good, but will need a rebase & a redo on the abi crate prob. will do so tomorrow and get this over the line cc @mattsse

@Evalir Evalir merged commit f23eb42 into foundry-rs:master Jun 23, 2023
@0xYYY
Copy link
Contributor Author

0xYYY commented Jun 24, 2023

thanks @Evalir for pushing this over the line

@DaniPopes DaniPopes mentioned this pull request Dec 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Evalir Evalir Evalir approved these changes

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unable to parse mnemonic in French
5 participants
@0xYYY @prestwich @Evalir @mattsse @DaniPopes