Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Use newer ethereumjs-util #5261

Merged
merged 7 commits into from Jul 11, 2022
Merged

Use newer ethereumjs-util #5261

merged 7 commits into from Jul 11, 2022

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Jul 5, 2022

Hopefully fixes #5260

@gnidan
Copy link
Contributor

gnidan commented Jul 5, 2022

Hey @wbt thanks for this! Looks like you need to commit the yarn.lock changes (in addition to whatever that other failure is)

@wbt
Copy link
Contributor Author

wbt commented Jul 5, 2022

Yeah, it looks like it might actually be more involved with having to adapt to some breaking changes in the dependency.
Unfortunately, I don't have the bandwidth to take that deep dive in right now.

@wbt
Copy link
Contributor Author

wbt commented Jul 5, 2022

The changelog about known-breaking changes is here.

@gnidan
Copy link
Contributor

gnidan commented Jul 5, 2022

Yeah, it looks like it might actually be more involved with having to adapt to some breaking changes in the dependency.
Unfortunately, I don't have the bandwidth to take that deep dive in right now.

dang! maybe someone else will be able to pick this up quickly. anyway, always appreciate your contributions in any capacity 🙏

@eggplantzzz
Copy link
Contributor

So it looks like there was some issue where things that were "mnemonic-like" but invalid were parsed as private keys. I updated the parsing in hdwallet-provider to fix this issue and resolve the test that was failing.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Looks good, tests are passing. I have a question about sniffing out a private-key.

packages/hdwallet-provider/src/constructor/getOptions.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

I think there's a bug in this relating to the private key parsing. I also have some style comments.

packages/hdwallet-provider/src/constructor/getOptions.ts Outdated Show resolved Hide resolved
packages/hdwallet-provider/src/constructor/getOptions.ts Outdated Show resolved Hide resolved
packages/hdwallet-provider/src/constructor/getOptions.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Hitting approve on this, seems good to me! I think isMnemonicLike would be a better name than isMnemonicPhrase but now that there's a least some validation it doesn't bother me. :P

@haltman-at
Copy link
Contributor

Oh, wait, this is failing CI for some reason...

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

LGTM pending tests ofc :)

@cds-amal
Copy link
Member

The updated function names are much better.

@eggplantzzz eggplantzzz merged commit bee96bf into trufflesuite:develop Jul 11, 2022
@wbt wbt deleted the patch-2 branch July 13, 2022 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hdwallet-provider uses old ethereumjs-util
5 participants