Showing clearer data in the personal_sign signature

[bwheeler96]

Can we make the renderBody method configurable for personal_sign?

Without opening up the whole "how should off-chain data get signed" can of worms, it would be great if personal_sign could be configured in the RPC call to simply show the hex data rather than the utf8 encoded version. I'd rather have my users see 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470 instead of :&�K"b��>7� SS�/���Ļ�)֙�.�I��

Basically as a developer I have three options in my smart contracts

1. Develop for eth_sign which has a very mean looking warning 2.Develop for personal_sign which also looks scary to users when utf8 encoded
2. Use signTypedData which is fantastic, but not supported by any other clients. I want to avoid designing an immutable contract to work with a spec that is only 50% supported. I could design support for both signTypedData and the traditional method, but now my smart contract needs to be aware of the different signing clients. Also not preferred.

I'm not looking to fix signing for the whole ecosystem, but if we could just make it a little more configurable in MetaMask for the time being that would be really cool.

https://github.com/MetaMask/metamask-extension/blob/9d4be1842e7c56e3bfde529ff555dcae8dec3dbd/ui/app/components/signature-request.js#L161

[nikitaeverywhere]

This issue should be easy to fix: https://github.com/MetaMask/metamask-extension/blob/9d4be1842e7c56e3bfde529ff555dcae8dec3dbd/ui/app/components/signature-request.js#L169 (I suppose in this.msgHexToText(data) simply check whether data begins with 0x and if it is, do msgHexToText otherwise not).

Look, the deprecated sign does this in a better way:

@danfinlay this should be very easy to fix, isn't it?

[bwheeler96]

@danfinlay if you think this is qualifies as an issue that should be fixed, I'm happy to make a fix. I think that signing as hex should be available as a parameter, the same way web3.sha3 accepts { encoding: 'hex' } as an argument.

[danfinlay]

Look, the deprecated sign does this in a better way:

Deprecated sign only shows hex ;)

I'm happy to make a fix.

I think your proposed solution is fine, and simple enough that I'd welcome you to make a PR.

[kumavis]

@bwheeler96 do any other clients support the encoding param?

[nikitaeverywhere]

Or, what do you think about the idea of automatic encoding detection? This should be easy to check: if the data to sign has any non-printable characters then display it accordingly. On Tue, Apr 24, 2018 at 7:26 PM kumavis [email protected] wrote:

@bwheeler96 https://github.com/bwheeler96 do any other clients support the encoding param?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MetaMask/metamask-extension/issues/3931#issuecomment-383995362, or mute the thread https://github.com/notifications/unsubscribe-auth/AEwhSGQkQ5aWcEoWDk_HxQFJMegErraaks5tr1IsgaJpZM4TLqj2 .

[kumavis]

@ZitRos how would you detect binary vs maybe chinese

[bwheeler96]

@ZitRos I am personally opposed to the idea of implicit data type detection. However having a console warning if the data "looks like" it was signed using the wrong encoding is probably a good idea.

@kumavis I don't think so. I'm trying to see what default encoding Parity uses, IIRC they show hex for both but I can't get their new client running.

At this point there are a lot of competing philosophies for wallets design, with the prevailing sentiment being a very conservative "let's do nothing until everyone agrees". As a developer I would rather be required to code separately for a handful of different implementations with a good user experience today then have my UX suffer for years until we can make a unicorn standard that satisfies every stakeholder. (see: EIP712, and the lack of implementation on other clients).

I think whether we like it or not we're entering a brand new browser wars. Let's just embrace it.

[nikitaeverywhere]

@kumavis, you right that’s not easy in the case of chinese. Maybe then just let the user of Metamask chose between ascii and hex like Brian suggests? I think hex encoding will be dominant until EIP712 becomes mainstream (personally I am adopting EIP712 standard in my project, it is awesome).

On Tue, Apr 24, 2018 at 7:49 PM kumavis [email protected] wrote:

@ZitRos https://github.com/ZitRos how would you detect binary vs maybe chinese

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/MetaMask/metamask-extension/issues/3931#issuecomment-384002521, or mute the thread https://github.com/notifications/unsubscribe-auth/AEwhSPr_bl3EFmFKS6GuCSWSQm31iBlkks5tr1esgaJpZM4TLqj2 .

[j0xhn]

@bwheeler96 I see your branch was closed. Am I understanding correctly that your code was never merged in for this? Because trying now but not getting the desired results. Is there another workaround that you have found for better formatting?

[bwheeler96]

@johndangerstorey nope you are out of luck. No way to sign hex and have it appear as hex AFAIK.

[NoahZinsmeister]

See #5878 for a PR addressing this issue.

[Gudahtt]

It looks as though this was mostly addressed in #5878. Is an explicit encoding still needed at this point? I'm not sure how commonly this is encountered now that the 32-byte string case is dealt with.

[philipcmonk]

For our use it would still be nice, since we have the user sign a longer hex string, but it's still somewhat legible as a hex string (mainly, you can see the address you're sending to). We could ascii-encode the hex string, but then the verifier also has to ascii-encode, which is complexity we'd rather not include.

I share the aversion to implicit encoding detection. An "encoding" argument would work fine for us, but the easiest solution might be to show both the hex and the unicode interpretation.

[vandan]

What if we explicitly display both UTF-8 and raw hex string versions of the message they are being asked to sign within the wallet? @bschorchit should we simply encourage use of signTypedData_v4 method?

[bschorchit]

@vandan could you explain to me the advantages of displaying both? I've read through the comments but don't fully understand it.

Also the comment initially made on lack of wallet adoption for sign type data probably no longer applies. I do think we should lean towards encouraging the use of it instead of improving other methods, but happy to consider this if there's a good reason for it.

[vandan]

@bschorchit - I believe the advantage would be that displaying both avoids the need for implicit data type detection. @BelfordZ can add more detail there. People may also want to sign a message that is not in UTF-8. These are the two encodings that would be most likely to come through (UTF-8 & raw hex).

In general though, I agree that the developers should be encouraged to adopt: signTypedData_v4, which represents the latest version of the EIP-712 spec.

@bschorchit I'm still curious if you think the solution suggested here might help end-users get better messaging for dapps that have not adopted signTypedData_v4?