Personal API methods

[pkieltyka]

Hey @ricmoo just wondering if you've considered adding the personal api methods such as personal_sign among others (https://github.com/ethereum/go-ethereum/wiki/Management-APIs#list-of-management-apis) for the purposes of signing arbitrary data from a wallet.

This is something well supported by web3, ethjs and MetaMask.

FYI as well, I've uncovered an EIP that's a standard for machine-verifiable and human-readable typed data signing with Ethereum keys. MetaMask and Cipher have added support for the new method already called eth_signTypedData see: ethereum/EIPs#712

so two questions,

1. what do you think about the personal API as per the go-ethereum wiki?
2. what do you think about adding the experimental method, but supported by major wallets/clients, eth_signTypedData ?

[ricmoo]

Personal signing is fully supported in ethers.js, but you do not need a provider, it works client-side, wallet.sign(message) will provide you the signed message, compatible with geth and solidity (to make it compatible with parity, subtract 27 from the last byte). If you look at EthersWallet-iOS, it is shimmed into the personal object, so it works fine there too. It will be added to ethers.io in the next version (I need it for people to sign their messages to claim their Firefly kits).

I haven't seen that EIP, I will check it out. It is certainly something I have wanted. :)

[pkieltyka]

@ricmoo awesome. I knew you'd have it figured out :) thanks.

it makes sense that its client-side, but in the case of MetaMask which holds the private key, it requires the user to "sign" with a button click, I assume the MetaMask wallet provides a provider over some transport (postMessage?) at which point we get the signed data back.

My purposes is of course to use MetaMask, so I'll read the ethers-web3.js in the iOS client and hopefully it has the personal method there too. I'm excited for when this is pushed to ethers.js repo.

[pkieltyka]

does this look correct?

// if web3.currentProvider is ethers.. then..
const ethersBridge = web3.currentProvider
ethersBridge.sendAsync({ params: [utils.hexlify(utils.toUtf8Bytes('mymessage')), '0xADDY'], callback}

looks like from source, I need to hexlify the message string by hand like so.

it would be nice eventually in the bridge to have functions/methods that look more like: ethersBridge.personalSign(messageString, address)

[ricmoo]

The Web3BridgeProvider may not work out-of-the box, I'll be adding it (hopefully) soon, in a way that will work without any mangling. But the idea is to use it like a Web3 provider, which allows existing Web3 apps to work instantly, by swapping out the provider with an ethers provider:

var web3 = new Web3(new Web3BridgeProvider(provider));
web3.personal.signMessage(...)

By using the official Web3 object, it remains bug-for-bug compatible, as well as some of the weirder choices Web3 made. :)

That said, I'm hoping more people will instead use the RemoteSigner(window.currentProvider) which wraps the web3 provider with the ethers Signer interface.

These are mostly done, but require testing. This week I'm focusing more on the Firefly firmware, which should hopefully be done soon, for the v0 version, at least.

[pkieltyka]

@ricmoo personally I don't want to have any of the web3 or ethereumjs dependencies anywhere in my project. Dapps should build with the lib they choose, and most likely that will be the case. Ie. I don't see a lot of benefit to use the ethers Web3BridgeProvider for a 100% compat API which still requires the web3 dependency, the only advantage would be some kind of migration I suppose, but still I'm not sure that'd speed up much migration. web3 and ethereumjs pkgs are quite a mess, and dont follow conventions at times, and further their filesize is quite large (>500kb minified and 1.8mb unminified).

I plan to use Ethers directly and just use the small web3.currentProvider surface api with ethers directly.

Looking at the source of ethers-web3.js, I believe EthersBridgeProvider is sufficient, it could just use some extra helper functions to call the rpc methods.

[pkieltyka]

I think the right API is similar to as shown here: https://github.com/MetaMask/faq/blob/master/detecting_metamask.md#using-a-convenience-library

for example.. (without any need for a web3 dependency aside from the injected .currentProvider):

var ethers = require('ethers');
var provider = new ethers.providers.Web3BridgeProvider(web3.currentProvider);

provider.getBalance(address).then(function(balance) { ... });
provider.personalSign(msg, address).then(...); // which doesn't exist yet in ethers provider
provider.signTypedData(msgParams, address).then(...); // new as per EIP above but supported by MM

also FYI, see https://github.com/danfinlay/js-eth-personal-sign-examples relating to examples of ethSign, personalSign and ethSignTypedData written by a MetaMask dev

[ricmoo]

Right. What you want is what will be the Web3Provider, which allows you to wrap a Web3 provider and expose it as a ethers provider. The Web3BridgeProvider is for the other direction.

I expect the Web3Provider to only take about an hour or so to write (since it will inherit from JsonRpcProvider, but with its send overridden), and many of the existing test cases should carry over.

The Web3Provider will also have a getSigner method, which will give you an object that behaves like a Wallet, which has signMessage, send, getBalance and all that jazz, but connected to Metamask or whatever the currentProvider is talking to. I need to get a bit done on EthersWallet, but I will try to get some of this done today as well.

I'm thinking of doing the conversion internally of signed messages too, so if you call parity or get you get the same result (a signature ending in 0x1b or 0x1c)... I don't want things to change in the front-end because the backend changes. Feedback?

[pkieltyka]

@ricmoo that sounds great. Nice and lean, and makes ethers.js quickly useable with the injected web3.currentProvider, as a simple integration.

Question, web3.currentProvider offers send() and sendAsync() - which is obvious as synchronous (wait for response) and asynchronous (pass a callback or promise). I noticed the send() method in ethers returns a promise, so I believe you'll want it to call web3.currentProvider.sendAsync(). I am not sure when you'd use a sync operation ever ..?

I do like the convenience of the signer too, that will be helpful but I don't believe the currentProvider does anything with signing, it just relays the rpc request/response straight up, but you probably know that so maybe I'm not understanding the difference other than you mean, the methods on the Web3Provider such as signMessage(), getBalance() etc. will take domain-level variables such as a plain text message for signMessage and the hex account number, and the Web3Provider will handle encoding or signing - which yes I certainly agree that you should cover those higher-level methods in the domain so app devs dont have to think about encoding or signing. But of course you should make the .send() (or sendAsync()) available, so if someone wants to call an RPC method that isn't included in the ethers Web3Provider, they can still get by.

I was not aware that parity or geth generate different signatures, is this related to the -27 last byte thing? which I've yet to fully dive in to understand why this has occurred/remains. I do agree all request/response from an app perspective, regardless of backend should be consistent. However, hopefully this doesnt have to be too hackey either or it'll make a mess of your lib code.

[ricmoo]

Correct, my send is async (as ethers does not support sync in any way and never will), so I will override the JsonRpcProvider.send in the subclass Web3Provider to call the web3Provider's sendAsync. No worries, it's simple and I've done it before for simple scripts. :)

I don't want to conflate what a Provider vs a Signer is. A Signer encapsulates private methods (like sign, sendTransaction, and signMessage) and can be passed into other libraries (such as ethers-meow, ethers-ens and will have the necessary properties to sign. A Provider only exposes public methods (like getLogs, getTransaction).

The Signer returned by the Web3Provider will simple proxy the commands through to the RPC. But keeps the two concepts completely separated, so that anything that takes in a provider will continue to work with any other provider and anything that takes in a Signer will continue to work with any other Signer (e.g. in the future, there will likely be a LedgerSigner and a FireflySigner).

The only reason is for encapsulation.

The send is still available on a Web3Provider, which takes in a method and params. So if you do want to call the platform specific calls, you can. (e.g. 'parity_addNode').

Yes, it's the -27 thing... The best way I can describe Parity is that it does what the right thing is, that should have been the standard, instead of the standard thing. Somewhat annoying, but maybe not wrong?

[pkieltyka]

+1 for encapsulation & separation of concerns, that makes lots of sense.

Signer is for methods using a wallet's private key, and Provider is for the network - do I have that right?

I'm really enjoying working with ethers btw, thanks for this dope project. I look forward to one day seeing this thing in TypeScript and having type safety + hinting, ahhh dreams

[ricmoo]

Yupp, absolutely correct.

I have finished the Web3Provider and am just running test cases now... Maybe I can get some quick feedback? :)

See: 67bb0b7

[pkieltyka]

@ricmoo nice work dude!

here's some questions/feedback:

1. MetaMask posts a warning about the use of eth_sign, but its fine with personal_sign and the methods are almost identical: https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign and https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign - can you add support for personal_sign, perhaps call the function, signer.personalSignMessage(msg) ? this should work: return provider.send('personal_sign', [ utils.hexlify(data), address, password ]);

2. Are the request id's in the payload supposed to be unique per connection? I'm not clear from the server spec yet how the id's are treated, ie. https://github.com/ethers-io/ethers.js/blob/master/providers/web3-provider.js#L139 you hardcode the id to 42, why is that?

I'll give this a shot in our dapp tomm morning and replace the web3 provider with the new ethers Web3Provider - thanks very much!

[pkieltyka]

the only other feedback I have in general is to perhaps name the functions on the provider similarly to the names of the RPC methods where the eth is a default preview but anything from the personal would explicitly call out the personal method. therefore, under that convention you'd probably rename provider.signMessage(message) to provider.sign(message) and then have provider.personalSign(message) and provider.personalUnlock(password) .. just a thought anyways about working around a convention to the method names of the RPC spec/methods

[ricmoo]

I've noticed that too, do you have any information why Metamask has that notice? It seems to me the eth_sign and personal_sign perform the same operation. The problem is that personal_sign doesn't exist in Parity. I can try personal_sign and fallback onto eth_sign if it is missing, but that seems sloppy... I could also detect Metamask and use personal_sign instead only for Metamask.

The id is completely arbitrary. It doesn't matter what it is, it just has to be something unique per connection, but each request only lives for one connection. It's only really useful in a long-lived connection to a JSON-RPC connection (e.g. WebSockets), as it allows the sender to multiplex and send multiple requests and know which request each response corresponds to. I usually choose 42 since it is a number that is somewhat obvious and uncommon, so if I see it show up in logs/debugging info somewhere, it rings a bell in my head saying "Hmmm... I am probably responsible for this"... :)

[shazow]

Re: eth_sign vs personal_sign on Metamask: https://medium.com/metamask/the-new-secure-way-to-sign-data-in-your-browser-6af9dd2a1527

Basically eth_sign is deprecated for being insecure, and has been replaced as an alias for personal_sign until the migration is complete (not sure why they went with this deprecation strategy, I guess at least it's still possible to ecrecover with an added prefix).

Parity should have personal_sign as of last week: openethereum/parity-ethereum#7453

[pkieltyka]

yea, it seems since the eth_sign behaviour changed to the more secure version with the prefix, Metamask is posting a warning in case someone is executing some old version. The sensible thing to do is just use personal_sign with the signer. When would someone call eth_sign or personal_sign on a provider anyways, as they'd need the private key?

@ricmoo you could leave it up to the dapp dev to make their choice between eth_sign or personal_sign by offering both without any fallback. For example, ethereumjs-testrpc aka ganache does not implement personal_sign, but it does implement the latest behaviour of eth_sign (which is the same), so in my dapp tests, I make a json-rpc call to web3_clientversion, memoize this value, if the client is testrpc then my method calls eth_sign, otherwise calls personal_sign as it expects a browser wallet such as MetaMask which should have personal_sign.

you could do something similar if you wanted to, and have signMessage a high-level method that checks a client version (memoizes it), and does the "intended" thing. But, you should still have all low-level methods available on the client for devs to have the power to execute whichever method they want by their own logic.

[ricmoo]

@shazow For now though, many people (e.g. me :) ) don't have it, and it still isn't added to their list: https://paritytech.github.io/wiki/JSONRPC-personal-module.html

What is the difference though? If they both behave identically, how is one secure and the other not?

For now, I will do what I can to make sure it just works. A person should not care what they are connected to, Parity JSON-RPC, Geth JSON-RPC, Metamask, etc. I think I will use the .isMetamask that Metamask mock Web3 providers inject, and for those use personal_sign. As a developer, you shouldn't need to care, just ask the library to sign a message. :)

[ricmoo]

@pkieltyka Well, that is my annoyance with the Web3 API. It conflates provider and key. eth_accounts, eth_sign and eth_sendTransaction require the private keys (or specific knowledge of them in some way).

And you shouldn't need to call web_clientVersion; that's insane. Which is what ethers.js attempts to do, provide a concise, complete interface to Ethereum, regardless of the backend.

Which is also why I don't think the API should be bogged down with all the various endpoints that various implementations implement, a person who wrote an app against a Web3Provider and Web3Signer should be able to trivially switch to an InfuraProvider and a Wallet. If you use web3Provider.sendTransaction(tx), in the future if you move from Web3 to local private keys, wallet.send(tx) does the exact same thing.

There is a way to do it if they want, but the 99% use case is covered.

// If someone *really* wants a command not offered by the API (because it is
// node specific, obscure or non-standard)
web3Provider.send('parity_killAccount', [ address, password ]).then(function(result) {
    console.log('Success:', result);
})

Anyhoo... Did you get a chance to try it out? I'm going to upload the new version with Web3Provider to the CDN tonight.

[shazow]

@ricmoo

What is the difference though? If they both behave identically, how is one secure and the other not?

Before eth_sign was deprecated, it had different behaviour. It would sign arbitrary bytes (no forced prefix), so you could trick people into signing transactions. Now that it's deprecated, its functionality was changed to be identical to personal_sign as part of the transition.

[ricmoo]

Oh, so eth_sign, once up a time, used to not prepend the message prefix... That is indeed super unsafe (I can just compute the unsigned hash of a transaction and ask it to be signed as a message and then re-serialize it). But since that has changed completely non-backwards compatibly, I don't understand why the move to personal_.

The Wallet.signMessage has always prepended a message prefix.

[ricmoo]

@shazow Yes, I see now. Thanks. :)

But the current behaviour isn't insecure. And the old behaviour is no longer possible. So, why would they deprecate it? And move it?

[shazow]

@ricmoo Yea, I don't fully understand the deprecation/migration strategy. I think the idea is that they wanted to keep eth_sign because at least DApps would still work in a recoverable way for now, but DApps that have migrated can switch to personal_sign with some stability guarantees.

[pkieltyka]

I believe MetaMask did the wrong thing by suggesting the warning even after fixing the eth_sign vulnerability after the change to protocol change of eth_sign with the message prefix. I am sure MetaMask will fix this in future versions, but given that many users running older versions of MM will have that waiting, its best to test for isMetaMask and call personal_sign instead specifically for MM.

[shazow]

I feel like it makes sense to use personal_sign when it's available and issue a warning if you need to fallback to something else that is deprecated (eth_sign). IIRC they're adding more stuff into the personal_* namespace too.

[pkieltyka]

@ricmoo I'm receiving the following error when trying to instantiate the signer,

TypeError: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, #<Object> at Function.defineProperty (<anonymous>)

in

this.provider = new ethers.providers.Web3Provider(web3CurrentProvider)
this.signer = this.provider.getSigner() // error start here

my test case stack trace references https://github.com/ethers-io/ethers.js/blob/master/providers/web3-provider.js#L19

[ricmoo]

Sorry, yes, I've fixed that in my working copy.

For now you need to specify an address to signer. The fix will be up within an hour.

provider.listAccounts().then(function(accounts) {
    signer = provider.getSigner(accounts[0]);
});

[ricmoo]

I'm trying to find a definitive source whether personal_sign takes in [ address, message ] or [message, address ]. Both work in Metamask. :s

[ricmoo]

Found some sources. It is running through Travis CI right now. :)

[shazow]

@ricmoo In the spirit of "parity probably does the right thing", looks like [ message, address, password ].

[pkieltyka]

@ricmoo btw, i've moved my dapp to ethers and its working well! I made the fixes locally to move forward and I see you just committed the fixes as well.

The only other thing I ran up against is when running tests against ethereumjs-testrpc (aka ganache-core/cli), the test first grabbed eth_accounts, took the first one then did an eth_sign of a message. I received the error from testrpc that there was "no private key for this address". The reason is ethers returns the accounts in EIP55 checksum form, and testrpc does a string comparison to find a private key for an address.. soo.... I downcased the account when passing it to testrpc and it works. Lol, I will submit a PR to testrpc for this, along with the addition of support for personal_sign. But just a heads up for others.

[ricmoo]

@pkieltyka Ugh... Yes, good point, I should lowercase addresses in some of the calls. I had a similar issue when adding Web3 emulation to EthersWallet; CryptoKitties doesn't allow checksum addresses either.

[ricmoo]

Ok... One more round of Travis CI. :)

[pkieltyka]

@ricmoo congrats on the support for MetaMask and other web3.currentProvider-based wallets.

Can you push the dist files to npm release?

[ricmoo]

Ack! I published to my CDN but forgot npm.

Ok, published. [email protected].

Thanks! I hope this helps expand out the user base. :)

[shazow]

Woo! Going to try this again on my next project, thanks @ricmoo! :D

[pkieltyka]

@ricmoo the last recommendation if you want to expand the user base:

1. Update the README to explicitly tell users ethers works with MetaMask, and provide the small example how to use it, with a few method examples to get address, sign a message, and get balance
2. Create a ethers-io/examples repo with a bunch of mini dapp examples using ethers, this serves as a starter but also documentation "by example", targetting dapp's/browsers specifically