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

Bump eth-sig-util to 2.3.0 #6896

Merged
merged 2 commits into from
Jul 23, 2019
Merged

Conversation

whymarrh
Copy link
Contributor

Gudahtt
Gudahtt previously approved these changes Jul 23, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

On #6661, @danfinlay mentioned having to update the keyrings to use the new version of eth-sig-util as well. That hasn't been done yet - they're still on 1.x.

This change seems fine either way - we were already using 2.x. I'm wondering what the consequences of our dependencies using the older eth-sig-util might be though - whether any expectations about these signatures bleed across package boundaries. 🤔

bitpshr
bitpshr previously approved these changes Jul 23, 2019
Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

The reason this was listed as a breaking semver change (eth-sig-util@>=2.x.x is because the signTypedData method was assigned a new name. We should make sure we are pointing to the correct method in this PR:
https://github.com/MetaMask/eth-sig-util/pull/20/files

@danfinlay
Copy link
Contributor

Looks like Mark already raised my concern, feel free to dismiss my review, I just mean that we should make sure it still works the same, like on the signature test site:
https://danfinlay.github.io/js-eth-personal-sign-examples/

@whymarrh whymarrh dismissed stale reviews from bitpshr and Gudahtt via 856d71c July 23, 2019 19:05
@whymarrh
Copy link
Contributor Author

On #6661, @danfinlay mentioned having to update the keyrings to use the new version of eth-sig-util as well. That hasn't been done yet - they're still on 1.x.

Updated to bump eth-keyring-controller as well. It is a major version bump but looking at the commits it's all dependency version bumps.[1]

I believe this change now accomplishes what we need—I don't think the other instances of the module are important in this context. npm ls --production eth-sig-util:

├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected] 
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └── [email protected]  deduped
│ ├── [email protected] 
│ └─┬ [email protected]
│   └── [email protected]  deduped
├─┬ [email protected]
│ └── [email protected] 
├── [email protected] 
├─┬ [email protected]
│ └── [email protected] 
└─┬ [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected]  deduped
  │ ├── [email protected] 
  │ └─┬ [email protected]
  │   └── [email protected]  deduped
  ├── [email protected] 
  └─┬ UNMET DEPENDENCY web3-provider-engine@github:metamask/provider-engine#e91367bc2c2535fbf7add06244d9d4ec98620042
    └── UNMET DEPENDENCY [email protected] 

By module:

I'm wondering what the consequences of our dependencies using the older eth-sig-util might be though - whether any expectations about these signatures bleed across package boundaries. — @Gudahtt

That's a valid concern and something we should audit. I imagine we can/will run into issues with the version mix we have (e.g. if normalize were to behave differently across versions or something like that). A separate concern though, I believe.

The reason this was listed as a breaking semver change (eth-sig-util@>=2.x.x is because the signTypedData method was assigned a new name. We should make sure we are pointing to the correct method [...] — @danfinlay

The extension was already on 2.x so it shouldn't need undergo any changes for us to bump the direct dependency. Looking at the API for the KeyringController, bumping that as well shouldn't require extension changes.

That said, I did test this with the signature site and it looks good to me. Maybe @tmashuang can also give it his seal of approval? I've add @brunobar79 to the reviewers here as well for good measure.

@whymarrh whymarrh requested review from brunobar79 and tmashuang July 23, 2019 19:05
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me 👍

Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

Looks like @whymarrh did the due diligence, thanks!

@danfinlay danfinlay merged commit 2788a94 into MetaMask:develop Jul 23, 2019
@whymarrh whymarrh deleted the bump-eth-sig-util branch July 23, 2019 19:25
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.

4 participants