feat: Add message signing

[Schwartz10]

Fixes #124.

Changes

If you are modifying the external API of one of the modules, please remember to also change the documentation

• I have updated the associated documentation with my changes

[CLAassistant]

All committers have signed the CLA.

[Schwartz10]

Credit to @mul1sh for help on this

[Schwartz10]

Hey @sohkai this is ready for an initial review + I have a couple questions:

1. Do you think we should include the account that will be responsible for signing this message in the signatureBag? The wrapper doesn't need to send this information in order to execute the signature, but from might feel a little misleading (besides the fact that this pattern exists for the transactionBag as well). Happy to keep as is and make updates down the road if needed.
2. Take a look at the change made in this commit to aragon-api to handle errors: 0d9877d#diff-9007ed863cba4431686946c9026c1b9aR345

This isn't great, but I think it's better than plucking the result, because front-ends can more easily handle error cases. However, I'm wondering if there's a better way to accomplish the same goal. Ideally, front-ends should be able to do this:

aragonApi.signMessage((err, sigHash) => {
  if (err) // handle it
  // use sigHash
})

Because the error first callback style is familiar.

Instead, the rxjs allows a front-end dev to do this:

aragonApi.signMessage(response => {
  if (response.error) // handle it
  else // use response.result for sig hash
})

This is way less ideal because it's unfamiliar.

I'm an rxjs noob, so is there a better, simple way to imitate the normal web3 callback style using observables?

[Schwartz10]

Once we settle on the API interface from the comment above, I'll update the docs

[Schwartz10]

Linking aragonone/product#33

[sohkai]

1. Do you think we should include the account that will be responsible for signing this message in the signatureBag?

No, this shouldn't be necessary. An aragon client should be responsible for connecting to an Ethereum provider that provides the connected account that will sign the message.

This pattern exists for the transactionBag as well

The transactionBag is purely an aragon/aragon concept, but I'm not sure I understand what you mean.

from should always be the sender address of a transaction. We do a bit more processing in getTransactionPath() against all declared accounts to help users find a transaction path if they've connected more than one account with aragon.js (this never happens in aragon/aragon).

2. Take a look at the change made in this commit to aragon-api to handle errors

The promise returned from @aragon/wrapper's signMessage() can be rejected, which should end up rejecting on the @aragon/api caller's side as well. This is how an app would detect that their sign request was cancelled or had failed.

The rejected promise is not currently being propagated, but working to get it to work :).

[Schwartz10]

Hi @sohkai thank you for reviewing. I'm going to address all your comments (including the two inline ones) here. I want to make sure I'm following the desired logic properly.

aragon/aragon needs 3 pieces of information to execute a signature and follow the given designs:

1. the message to sign
2. the account to sign the message from
3. the app that requested the signature

(1) is easy to handle, but there are different options for how we handle (2) and (3) with respect to getting and passing the data (between packages), so I'm going to go through both and make sure we're on the same page.

(2) - the account to sign the message from

An aragon client should be responsible for connecting to an Ethereum provider that provides the connected account that will sign the message.

I see three ways to achieve this:

a. The aragon client app developer could pass the account to sign from as a second parameter in the call to aragon/api.requestSignMessage like: app.requestSignMessage(msg, account).
b. Once the call to aragon/api.requestSignMessage reaches aragon/wrapper, we could get the account to sign from through this.accounts. This would mean the call to app.requestSignMessage would not specify the account to sign from.
c. Once the call reaches aragon/aragon, we could use the walletWeb3 to get the current account (this also means that the call to app.requestSignMessage would not specify the account to sign from)

Which option do you think makes the most sense?

(3) - the app that requested the message signature

I'd just have signMessage(message) be the function signature

If I'm understanding correctly, you can access the app address that made the call to aragon/api through proxy.address here. Is this is the best place to grab the requesting app's address? If so, doesn't the signMessage signature need to include another param? If not, where is the best place to grab the requesting app address?

Lastly, about documentation:

The promise returned from @aragon/wrapper's signMessage() can be rejected, which should end up rejecting on the @aragon/api caller's side as well. This is how an app would detect that their sign request was cancelled or had failed.

So does this mean that the documentation should suggest promisifying the Observable returned by api.requestSignMessage, and wrapping it in a try catch (or using .then, .catch)?

[sohkai]

(2) - the account to sign the message from

Definitely c. This is something the client should handle itself, since it should own the connection to your "wallet" Ethereum provider.

The other two options would essentially be the same thing, since the same account is injected into @aragon/wrapper (and @aragon/api just pulls this account), but would introduce unnecessary clutter into the API.

(3) - the app that requested the message signature
Is this is the best place to grab the requesting app's address?

Yes, the handler is the best place to grab this address. There shouldn't be a need to include another parameter in requestSignMessage() on @aragon/api's side since all of this information is available in @aragon/wrapper.

So does this mean that the documentation should suggest promisifying the Observable returned by api.requestSignMessage, and wrapping it in a try catch (or using .then, .catch)?

There is technically some other development work that needs to be done too, to get this promise accept / reject. You can basically duplicate the pattern here or here, where the emitted object just holds an resolve and reject key that will be handled by the client.

The propagation of this error has been implemented in #277.

As for the documentation itself, you can use some of the wording from https://github.com/aragon/aragon.js/blob/master/docs/APP.md#requestaddressidentitymodification 😄.

[Schwartz10]

Definitely c.

Understood.

Yes, the handler is the best place to grab this address. There shouldn't be a need to include another parameter in requestSignMessage() on @aragon/api's side since all of this information is available in @aragon/wrapper.

Also understood. requestSignMessage (in aragon/api) does only take one param. However, your comment here says we should only need one parameter on the aragon/wrapper method. I think we're on the same page here, so take a look at the current code and let me know what you think.

The propagation of this error has been implemented in

Great, when i pulled in #277 this is working well. Although I think for documentation purposes it makes sense to suggest promisifying the requestSignMessage because you can handle the errors easily in a catch block (unless there's a good way to handle errors in a subscribe operator?). I updated the docs, so you can let me know what you think.

--
If the changes are good, I think it's ready to be pulled in!

[sohkai]

unless there's a good way to handle errors in a subscribe operator?

Yes, it's the second callback to .subscribe() 😄 .

[Schwartz10]

Yes, it's the second callback to .subscribe() 😄 .

Ha, realized I had tried the second error parameter before #277 was merged. Oops, brain fart.

I'll update documentation too

[Schwartz10]

@sohkai changed variable name to requestingApp. Hopefully this is all set now!

[sohkai]

Looking good, thanks @Schwartz10!

[2color]

Nice addition. Thanks @Schwartz10

[Schwartz10]

Hey thanks for wrapping this up @2color and @sohkai ! Very much appreciated.