bump eth-sig-util version to 5.0.2, stop inheriting from eth-simple-keyring
#70
Conversation
adonesky1
changed the title
eth-simple-keyring & remove ethereumjs-wallet
adonesky1
force-pushed
the
bump-eth-sig-util
branch
3 times, most recently
from
1f5074e
to
37dc86d
Compare
There was a problem hiding this comment.
Hey @adonesky1 I know this is in draft but I was curious about a couple of things:
- I can understand the motivation for removing the dependency on
eth-simple-keyring, but what do we feel about defining some kind of interface that all Keyring classes must follow? - Do we plan on converting this repo to TypeScript at any point? I guess that could solve the previous point.
yes for sure! Per my comment here, we will be creating an abstract class/type that will define base/required method signatures for all keyring types @mcmire |
|
@adonesky1 Ah sorry missed that. Makes sense! |
adonesky1
force-pushed
the
bump-eth-sig-util
branch
from
f51d40b
to
7a17681
Compare
adonesky1
changed the title
eth-simple-keyring & remove ethereumjs-walleteth-sig-util version to 5.0.2, stop inheriting from eth-simple-keyring & remove ethereumjs-wallet
|
Would it be possible to split this into 2 or 3 PRs? It does seem like three distinct changes. Maybe they're tied together in a way that isn't clear |
|
@Gudahtt no I think you're right. I let scope creep a ton here. Will break up into 2 or 3 PRs |
adonesky1
force-pushed
the
bump-eth-sig-util
branch
2 times, most recently
from
c5f2610
to
1bd2bd3
Compare
adonesky1
changed the title
eth-sig-util version to 5.0.2, stop inheriting from eth-simple-keyring & remove ethereumjs-walleteth-sig-util version to 5.0.2, stop inheriting from eth-simple-keyring
|
@Gudahtt this is now just bumping |
There was a problem hiding this comment.
Thanks for taking care of this! This should make the TypeScript conversion easier.
It was a bit hard to follow how this compared to eth-simple-keyring@4.2.0 because it seems like some changes were brought from the latest version of that package, and some were from v4.2.0. For example, all of the async functions are now using async instead of returning things wrapped in Promise.resolve.
So far I've made a lot of notes of changes to document in the changelog. There are a few things I still need to review more closely, I'll try to finish this review soon.
|
|
||
| // Options: | ||
| const hdPathString = `m/44'/60'/0'/0`; | ||
| const type = 'HD Key Tree'; | ||
|
|
||
| class HdKeyring extends SimpleKeyring { | ||
| class HdKeyring { |
There was a problem hiding this comment.
We're dropping EventEmitter here as well.
Seems appropriate, we aren't using any event emitter functionality here. Just noting it for the changelog.
| } | ||
|
|
||
| // For eth_sign, we need to sign arbitrary data: | ||
| async signMessage(address, data, opts = {}) { |
There was a problem hiding this comment.
Looks like we dropped the newGethSignMessage method as well. This seems fine too, just noting for the changelog
| } | ||
|
|
||
| // personal_signTypedData, signs data along with the schema | ||
| async signTypedData( |
There was a problem hiding this comment.
Looks like the signTypedData_v{1,3,4} methods have been omitted. This seems fine, just noting for the changelog.
| return wallet.privateKey; | ||
| } | ||
|
|
||
| _getWalletForAccount(account, opts = {}) { |
There was a problem hiding this comment.
Noting for other reviewers, this method has changed from returning a Wallet instance to returning an object with privateKey and publicKey properties. It seems everything using this has been updated to expect this though.
There was a problem hiding this comment.
It looks like this change was what brought in the ethereum-cryptography dependency as well. Was this leftover from the ethereumjs-wallet replacement work?
There was a problem hiding this comment.
It looks like this change was what brought in the ethereum-cryptography dependency as well. Was this leftover from the ethereumjs-wallet replacement work?
yes good catch
There was a problem hiding this comment.
Well actually we want to use ethereum-cryptography's version of keccak256 rather than ethereumjs-util's. This change was made in eth-simple-keyring here
| return publicKey; | ||
| } | ||
|
|
||
| _getPrivateKeyFor(address, opts = {}) { |
There was a problem hiding this comment.
Worth noting that this was renamed as well, to have an underscore
@Gudahtt Could you point out which bits you are seeing that are |
|
Oh, maybe it is all from v5. I thought it was a mix because I was comparing it to v4.2.0 and noticed the order the functions matched v4.2.0 more closely than the main branch, I didn't look at v5 directly. |
adonesky1
force-pushed
the
bump-eth-sig-util
branch
from
e6a04ab
to
d0c577e
Compare
Bumps version of
eth-sig-util(now@metamask/eth-sig-util) to v5.0.2.Removes inheritance of
eth-simple-keyringas we want to reorganize the relationships of various keyrings. The new structure will be that all keyrings, once converted to typescript, implement a base-keyring type which indicates the required method signatures to be a valid keyring. Given that this PR disentangleseth-simple-keyringfrometh-hd-keyringand the latter no longer inherits methods from the former (getAppKeyAddress,exportAccount,signTransaction,signMessage,signPersonalMessage,decryptMessage,signTypedData,removeAccount,getEncryptionPublicKey) the methods are now implemented oneth-hd-keyringnow too.