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

bump eth-sig-util version to 5.0.2, stop inheriting from eth-simple-keyring #70

Merged
merged 11 commits into from
Dec 6, 2022
148 changes: 143 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,33 @@
const { hdkey } = require('ethereumjs-wallet');
const SimpleKeyring = require('@metamask/eth-simple-keyring');
const { keccak256 } = require('ethereum-cryptography/keccak');
const {
stripHexPrefix,
privateToPublic,
publicToAddress,
ecsign,
arrToBufArr,
} = require('@ethereumjs/util');
const bip39 = require('@metamask/scure-bip39');
const { wordlist } = require('@metamask/scure-bip39/dist/wordlists/english');
const { normalize } = require('@metamask/eth-sig-util');
const {
concatSig,
decrypt,
getEncryptionPublicKey,
normalize,
personalSign,
signTypedData,
SignTypedDataVersion,
} = require('@metamask/eth-sig-util');

// Options:
const hdPathString = `m/44'/60'/0'/0`;
const type = 'HD Key Tree';

class HdKeyring extends SimpleKeyring {
class HdKeyring {
Copy link
Member

Choose a reason for hiding this comment

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

We're dropping EventEmitter here as well.

Seems appropriate, we aren't using any event emitter functionality here. Just noting it for the changelog.

/* PUBLIC METHODS */
constructor(opts = {}) {
super();
this.type = type;
this._wallets = [];
this.deserialize(opts);
}

Expand Down Expand Up @@ -126,7 +141,130 @@ class HdKeyring extends SimpleKeyring {
);
}

/* PRIVATE METHODS */
/* BASE KEYRING METHODS */

// returns an address specific to an app
async getAppKeyAddress(address, origin) {
if (!origin || typeof origin !== 'string') {
throw new Error(`'origin' must be a non-empty string`);
}
const wallet = this._getWalletForAccount(address, {
withAppKeyOrigin: origin,
});
const appKeyAddress = normalize(
publicToAddress(wallet.publicKey).toString('hex'),
);
return appKeyAddress;
}

// exportAccount should return a hex-encoded private key:
async exportAccount(address, opts = {}) {
const wallet = this._getWalletForAccount(address, opts);
return wallet.privateKey.toString('hex');
}

// tx is an instance of the ethereumjs-transaction class.
async signTransaction(address, tx, opts = {}) {
const privKey = this._getPrivateKeyFor(address, opts);
const signedTx = tx.sign(privKey);
// Newer versions of Ethereumjs-tx are immutable and return a new tx object
return signedTx === undefined ? tx : signedTx;
}

// For eth_sign, we need to sign arbitrary data:
async signMessage(address, data, opts = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we dropped the newGethSignMessage method as well. This seems fine too, just noting for the changelog

const message = stripHexPrefix(data);
const privKey = this._getPrivateKeyFor(address, opts);
const msgSig = ecsign(Buffer.from(message, 'hex'), privKey);
const rawMsgSig = concatSig(msgSig.v, msgSig.r, msgSig.s);
return rawMsgSig;
}

// For personal_sign, we need to prefix the message:
async signPersonalMessage(address, msgHex, opts = {}) {
const privKey = this._getPrivateKeyFor(address, opts);
const privateKey = Buffer.from(privKey, 'hex');
const sig = personalSign({ privateKey, data: msgHex });
return sig;
}

// For eth_decryptMessage:
async decryptMessage(withAccount, encryptedData) {
const wallet = this._getWalletForAccount(withAccount);
const { privateKey } = wallet;
const sig = decrypt({ privateKey, encryptedData });
return sig;
}

// personal_signTypedData, signs data along with the schema
async signTypedData(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the signTypedData_v{1,3,4} methods have been omitted. This seems fine, just noting for the changelog.

withAccount,
typedData,
opts = { version: SignTypedDataVersion.V1 },
) {
// Treat invalid versions as "V1"
const version = Object.keys(SignTypedDataVersion).includes(opts.version)
? opts.version
: SignTypedDataVersion.V1;

const privateKey = this._getPrivateKeyFor(withAccount, opts);
return signTypedData({ privateKey, data: typedData, version });
}

removeAccount(address) {
if (
!this._wallets
.map((w) => normalize(w.getAddress().toString('hex')))
.includes(address.toLowerCase())
) {
throw new Error(`Address ${address} not found in this keyring`);
}

this._wallets = this._wallets.filter(
(w) =>
normalize(w.getAddress().toString('hex')) !== address.toLowerCase(),
);
}

// get public key for nacl
async getEncryptionPublicKey(withAccount, opts = {}) {
const privKey = this._getPrivateKeyFor(withAccount, opts);
const publicKey = getEncryptionPublicKey(privKey);
return publicKey;
}

_getPrivateKeyFor(address, opts = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Worth noting that this was renamed as well, to have an underscore

if (!address) {
throw new Error('Must specify address.');
}
const wallet = this._getWalletForAccount(address, opts);
return wallet.privateKey;
}

_getWalletForAccount(account, opts = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this change was what brought in the ethereum-cryptography dependency as well. Was this leftover from the ethereumjs-wallet replacement work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const address = normalize(account);
let wallet = this._wallets.find((w) => {
return (
normalize(w.getAddress().toString('hex')) === address.toLowerCase()
);
});
if (!wallet) {
throw new Error('HD Keyring - Unable to find matching address.');
}

if (opts.withAppKeyOrigin) {
const { privateKey } = wallet;
const appKeyOriginBuffer = Buffer.from(opts.withAppKeyOrigin, 'utf8');
const appKeyBuffer = Buffer.concat([privateKey, appKeyOriginBuffer]);
const appKeyPrivateKey = arrToBufArr(keccak256(appKeyBuffer, 256));
const appKeyPublicKey = privateToPublic(appKeyPrivateKey);
wallet = { privateKey: appKeyPrivateKey, publicKey: appKeyPublicKey };
}

return wallet;
}

/* PRIVATE / UTILITY METHODS */

/**
* Sets appropriate properties for the keyring based on the given
Expand Down
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,14 @@
"test": "jest"
},
"dependencies": {
"@metamask/eth-sig-util": "^4.0.0",
"@metamask/eth-simple-keyring": "^5.0.0",
"@ethereumjs/util": "^8.0.2",
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
"@metamask/eth-sig-util": "^5.0.2",
"@metamask/scure-bip39": "^2.0.3",
"ethereum-cryptography": "^1.1.2",
"ethereumjs-wallet": "^1.0.1"
},
"devDependencies": {
"@ethereumjs/util": "^8.0.0-beta.3",
"@ethereumjs/tx": "^4.0.1",
"@lavamoat/allow-scripts": "^1.0.6",
"@metamask/auto-changelog": "^2.5.0",
"@metamask/bip39": "^4.0.0",
Expand Down
Loading