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.

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

feat: Add message signing #276

Merged
merged 15 commits into from Apr 13, 2019

Conversation

Projects
None yet
4 participants
@Schwartz10
Copy link
Contributor

Schwartz10 commented Apr 5, 2019

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

This comment has been minimized.

Copy link

CLAassistant commented Apr 5, 2019

CLA assistant check
All committers have signed the CLA.

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 5, 2019

Credit to @mul1sh for help on this

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 5, 2019

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 Schwartz10 force-pushed the openworklabs:feat/message-signing branch from b7e427d to 0d9877d Apr 5, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 5, 2019

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

@Schwartz10 Schwartz10 marked this pull request as ready for review Apr 5, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 5, 2019

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 8, 2019

  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).

  1. 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 :).

Show resolved Hide resolved packages/aragon-wrapper/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated

@sohkai sohkai changed the title add support for https://github.com/aragonone/product/issues/33 feat: Add message signing Apr 8, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Member

sohkai commented Apr 9, 2019

(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

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Member

sohkai commented Apr 9, 2019

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

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

@sohkai sohkai referenced this pull request Apr 9, 2019

Closed

WIP - Allow apps to sign data via a RPC call #261

1 of 1 task complete
@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 9, 2019

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

@sohkai

sohkai approved these changes Apr 10, 2019

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 10, 2019

Looking good, thanks @Schwartz10!

@2color

2color approved these changes Apr 10, 2019

Copy link
Contributor

2color left a comment

Nice addition. Thanks @Schwartz10

Update packages/aragon-wrapper/src/index.js
Co-Authored-By: Schwartz10 <Schwartz10@users.noreply.github.com>

sohkai added some commits Apr 11, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

Schwartz10 commented Apr 11, 2019

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

@sohkai sohkai merged commit 51b18bf into aragon:master Apr 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.