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 parameter ordering seems wrong #1557

Closed
kenshyx opened this issue Jun 7, 2017 · 3 comments · Fixed by MetaMask/web3-provider-engine#162
Closed

personal.sign parameter ordering seems wrong #1557

kenshyx opened this issue Jun 7, 2017 · 3 comments · Fixed by MetaMask/web3-provider-engine#162
Labels
area-provider Relating to the provider module. type-bug

Comments

@kenshyx
Copy link

kenshyx commented Jun 7, 2017

From this this and this it should be personal.sign(message, account, pwd). After playing a while with the web3 injected by metamask it seems it's using (account, message).

@danfinlay
Copy link
Contributor

danfinlay commented Jun 8, 2017

Thank you so much for catching this. This is pretty bad.

Thinking of ways to fix this while breaking the least possible. I was thinking we could try to respect existing behavior while moving forwards, like:

personalSign(account, message) {
  let correctAccount, correctMessage
  if ( resemblesMessage(account) && resemblesAccount(message) {
    console.warn(`Sorry developers, you should switch the param order, ${LINK_TO_BLOG_POST}`)
    correctAccount = message
    correctMessage = account
  }
  return oldPersonalSign(correctMessage, correctAccount)
}

Thoughts?

danfinlay added a commit to danfinlay/ethjs-schema that referenced this issue Jun 8, 2017
[It has been brought to my attention](MetaMask/metamask-extension#1557) that this (and in turn MetaMask's) personal_sign param ordering is incorrect.

How did this happen? Was it [the lack of a standard RPC test suite?](ethereum/EIPs#217) was it just my mistake? I'll claim both.

The Wiki seems to be accurate in correcting us here, too:
https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
@kumavis
Copy link
Member

kumavis commented Jun 8, 2017

That sounds pretty good Dan, what's the worst that could happen here? Accidently swapping the values if the message looks like an address?

@kumavis
Copy link
Member

kumavis commented Jun 8, 2017

I guess we could load the addresses and see if there's a match to tell the difference

@2-am-zzz 2-am-zzz added P2-sooner type-bug area-provider Relating to the provider module. labels Jun 12, 2017
SjonHortensius pushed a commit to SjonHortensius/ethjs-schema that referenced this issue Jul 31, 2017
[It has been brought to my attention](MetaMask/metamask-extension#1557) that this (and in turn MetaMask's) personal_sign param ordering is incorrect.

How did this happen? Was it [the lack of a standard RPC test suite?](ethereum/EIPs#217) was it just my mistake? I'll claim both.

The Wiki seems to be accurate in correcting us here, too:
https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-provider Relating to the provider module. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants