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

Implementation for eth_decryptMessage #7247

Closed
wants to merge 12 commits into from
Closed

Implementation for eth_decryptMessage #7247

wants to merge 12 commits into from

Conversation

logvik
Copy link
Contributor

@logvik logvik commented Oct 3, 2019

The new feature, as requested here #1190

logvik added 2 commits October 3, 2019 23:11
@logvik
Implementation for "eth_decryptMessage". It allows using private user…
5c6b117 
… keys via keyring and decrypting messages that were encrypted via eth-sig-util.

- Need to add translations for all languages, except en and ru
- Need to fix issue with badges. The decrypt request does not influence on badges, although code for this part is present. The core team, please review this piece.
@logvik
Merge branch 'develop' of https://github.com/logvik/metamask-extension …
bea6dae 
…into develop
@logvik logvik mentioned this pull request Oct 10, 2019
Closed
Gudahtt
Gudahtt reviewed Oct 17, 2019
View reviewed changes
app/manifest.json Outdated Show resolved Hide resolved
@logvik logvik requested a review from Gudahtt October 18, 2019 09:15
@whymarrh

This comment has been minimized.

Sign in to view
@whymarrh whymarrh self-assigned this Dec 3, 2019
@whymarrh whymarrh added this to the UI Sprint 24 [Nov 12] milestone Dec 3, 2019
@whymarrh
Copy link
Contributor

@logvik apologies for the delay in reviewing this—I've assigned myself here to indicate that I'll be helping get this merged. I will fix the conflicts here (most of them were from my work).

@logvik
Copy link
Contributor Author

logvik commented Jan 10, 2020

@whymarrh please don't waste time on this PR, I think we will be forced to close it because too many changes ware made in my repository since that moment when I opened this one. Please review #1190. There are some PR in other packages that block the new PR from my repository.

@whymarrh
Copy link
Contributor

Oh, sure, feel free to close this and I'll take a look at that one

@logvik
Copy link
Contributor Author

logvik commented Jan 13, 2020

@whymarrh , we have a blocked PR's, need to resolve them before I will prepare new PR for the extension:
MetaMask/KeyringController#50
MetaMask/eth-json-rpc-infura#15
MetaMask/eth-json-rpc-filters#11

@Gudahtt
Copy link
Member

Gudahtt commented Jan 13, 2020

@logvik The keyring package should probably be updated on those repos, sure, but I don't think those changes are blocking.

The version range of that package currently is inclusive of the new version, so we can use those packages as-is from metamask-extension with the new keyring without making any changes.

@logvik
Copy link
Contributor Author

logvik commented Jan 13, 2020

@Gudahtt I agree, it doesn't block the developing process. But for example, https://github.com/logvik/metamask-extension/blob/develop/package.json#L98 uses my repository with the updated version and due to this circumstance, I can't offer to merge it with metamask-extension.

@Gudahtt
Copy link
Member

Gudahtt commented Jan 13, 2020 •
edited
Loading

Yep, understood! What I mean is that you can use the real KeyringController with the latest version of eth-hd-keyring without publishing a new version of KeyringController, because the version ranges are compatible (KeyringController requires an eth-hd-keyring version of ^3.4.1, which is compatible with v3.5.0). I'd be happy to help with that part if you want - I can handle updating the lockfile to ensure only the new version is used.

@logvik
Copy link
Contributor Author

logvik commented Jan 14, 2020

@Gudahtt do you mean that in package.json add eth-hd-keyring with the new version and it will replace version into KeyringController? I have tried this way but for some reason, KeyringController uses the internal version. Also, I am not sure that I have "an allowance" to add some "duplicated" packages to package.json. Sorry in advance if I don't understand your advice again.

@Gudahtt
Copy link
Member

Gudahtt commented Jan 14, 2020

Yep, that's basically what I'm saying. It's understandable that you couldn't get it to work though, as it requires manual lockfile manipulation. If you get the rest of the branch updated, I can take care of that part and show you what I mean.

@logvik
Copy link
Contributor Author

logvik commented Jan 14, 2020

Maybe just simpler to ask team members to update npm and all? Then I will prepare new PR to the metamask-extension.

@whymarrh , we have a blocked PR's, need to resolve them before I will prepare new PR for the extension:
MetaMask/KeyringController#50
MetaMask/eth-json-rpc-infura#15
MetaMask/eth-json-rpc-filters#11

I can't reach out to @danfinlay. Usually, he did all merges.

@Gudahtt
Copy link
Member

Gudahtt commented Jan 14, 2020

Well, updating the lockfile is quite simple 😅 . I'm just not sure how to explain it except by example.
I just meant you don't need to consider yourself blocked; if you put up a PR that is ready apart from that one change, we can proceed.
But sure, I will also help get those changes published in the meantime as well.

@logvik logvik mentioned this pull request Jan 15, 2020
Merged
@logvik
Copy link
Contributor Author

logvik commented Jan 15, 2020

@Gudahtt , @whymarrh Migrated to #7831

@logvik logvik closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danjm danjm Awaiting requested review from danjm

@whymarrh whymarrh Awaiting requested review from whymarrh

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

Assignees

@whymarrh whymarrh

Labels
None yet
Projects
None yet
Milestone
UI Sprint 24 [Nov 12]
Development

Successfully merging this pull request may close these issues.
3 participants
@logvik @whymarrh @Gudahtt