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

personal_sign method does not work in status #11333

Closed
bgits opened this issue Oct 23, 2020 · 24 comments · Fixed by #11369
Closed

personal_sign method does not work in status #11333

bgits opened this issue Oct 23, 2020 · 24 comments · Fixed by #11369
Assignees

Comments

@bgits
Copy link

bgits commented Oct 23, 2020

Bug Report

Using the personal_sign method does not seem to work within Status while it works from web3 wallets.

Problem

Using the personal sign method fails to trigger a populated dialog that can be signed by the user.

An overview of the background required to understand the problem.

Attempting to use snapshot.page we discovered that Status is the only wallet unable to sign messages for voting.

A problem description.

Using the following test page: https://danfinlay.github.io/js-eth-personal-sign-examples/

When clicking on personal_sign it should trigger a populated dialog to sign (as in metamask). It either fails to trigger or populated an empty dialog box as when attempting to vote using snapshot.

Expected behavior

It should populate a dialog box and allow the user to sign it.

Actual behavior

Either no dialog box appears or an empty one sign request appears.

Reproduction

Additional Information

  • Operating System:
...
@bgits bgits added the bug label Oct 23, 2020
@flexsurfer
Copy link
Member

so there is the error ReferenceError: Can't find variable: web3 , status doesn't inject web3 library , it's not related , personal_sign works in status

@bgits
Copy link
Author

bgits commented Oct 26, 2020

Another way to recreate the error is using snapshot which is the primary use case that is being blocked by this issue now. It does trigger the signing box but it is empty.

You can recreate this error by going to:

https://demo.snapshot.page/#/status/create

and attempting to publish a poll by filling in all the fields.

@flexsurfer
Copy link
Member

@bgits it seems you're trying to sign typed data ? in that case you have to use a different method eth_signTypedData_v3

@bgits
Copy link
Author

bgits commented Oct 26, 2020

Here is another test: trying to sign both a plain text string and typed data works using metamask but neither works in Status.

https://gateway.pinata.cloud/ipfs/QmckPsNLbGSQjTTAwchqyrUrG7FwnrhdHsJh5n7gMn9ykV/

@flexsurfer
Copy link
Member

in this example sign message doesn't work because it uses eth.sign which is not supported, and for typed data it uses eth_signTypedData_v4 and status doesn't support it, we could add support for it if needed

@flexsurfer
Copy link
Member

do you know what's the difference between v3 and v4 and why you can't use v3 ?

@bgits
Copy link
Author

bgits commented Oct 27, 2020

do you know what's the difference between v3 and v4 and why you can't use v3 ?

It's being used because that is what ethers.js exposes. It does not exposes any other typed signer.

@bgits
Copy link
Author

bgits commented Oct 27, 2020

@bgits it seems you're trying to sign typed data ? in that case you have to use a different method eth_signTypedData_v3

I don't believe the error is due to typed data. The data being sent over is a JSON to sign but not typed data. Furthermore I created an additional test to use the same personal_sign method with a string and it produces the same error in Status.

updated test can be found here: https://gateway.pinata.cloud/ipfs/QmXECbBSchgQja9VQAqQopNTMwtwKcF46BEWrCgkfTJYGN/

@flexsurfer
Copy link
Member

we expect the message as a hex for personal_sign method

@flexsurfer
Copy link
Member

eth_signTypedData_v4 status-im/status-go#2062

@bgits
Copy link
Author

bgits commented Oct 27, 2020

we expect the message as a hex for personal_sign method

This is problematic because the user will not be able to understand what they are signing. How would signing status principles work for example?

This is also not consistent with how other wallets handle it.

@flexsurfer
Copy link
Member

flexsurfer commented Oct 27, 2020

This is problematic because the user will not be able to understand what they are signing.

we convert it to a string, all dapps sign fine in status, CK and others

This is also not consistent with how other wallets handle it.

it works according to spec in status https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign

@bgits
Copy link
Author

bgits commented Oct 27, 2020

it works according to spec in status https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign

I don't see anything in the spec that requires the message to be in hex. This is consistent with how every other wallet is handling it.

@bgits
Copy link
Author

bgits commented Oct 27, 2020

all dapps sign fine in status,

This is not true. This issue is open because https://snapshot.page/ does not sign in status while it does work with other wallets.

This is the most used voting dapp and we have been trying to use it with Status and are blocked by this.

@flexsurfer
Copy link
Member

then we need to implement something similar to MetaMask/metamask-extension#1177

@bgits
Copy link
Author

bgits commented Oct 27, 2020

we convert it to a string,

It also does not display hex as a string to the user. https://gateway.pinata.cloud/ipfs/QmVjDxwxTN4zDPDp9NzzANEEp5pUhNzKGUjHXFD9Sf1MRw/

@flexsurfer
Copy link
Member

flexsurfer commented Oct 27, 2020

It also does not display hex as a string to the user. https://gateway.pinata.cloud/ipfs/QmVjDxwxTN4zDPDp9NzzANEEp5pUhNzKGUjHXFD9Sf1MRw/

0x5445535420535452494e47000000000000000000000000000000000000000000, length: 66, text : TEST STRING

let's ask @richard-ramos about that
277f65b

@flexsurfer
Copy link
Member

it looks like its just will be better to implement it similar to MM

@flexsurfer
Copy link
Member

thanks @bgits

@richard-ramos
Copy link
Member

let's ask @richard-ramos about that
277f65b

I added that because back when I was working on teller network, the GSN made the users sign a random bytes32 hash that wasn't being shown at all in status, or displayed weird characters, and this way it would show the hex string (or at least did back then)

...

@bgits, how do you trigger the personal_sign to show the ascii for the messages when the lenght of the message is 32 bytes?
If I execute this on metamask i get the following result:

web3.eth.personal.sign("0x54455354204d4553534147450000000000000000000000000000000000000000", web3.eth.defaultAccount)

image

@bgits
Copy link
Author

bgits commented Oct 27, 2020

@bgits, how do you trigger the personal_sign to show the ascii for the messages when the lenght of the message is 32 bytes?

@richard-ramos https://github.com/status-im/ethers-testbed/blob/master/src/App.jsx#L68-L70

@cammellos
Copy link
Contributor

Regarding signtypeddata_v4 , I have noticed that metamask might not be according to specs:

MetaMask/eth-sig-util#106

MetaMask/eth-sig-util#107

So before we proceed any further it would be good to clarify exactly which one is the correct behavior.

We will also look at personal_sign.

@flexsurfer
Copy link
Member

same for personal_sign , there is no spec for signing utf8 strings, metamask just did their own implementation

@richard-ramos
Copy link
Member

On desktop for personal_sign what I did was check if the input value is a hex string, and if it's not, convert it to hex before signing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants