-
Notifications
You must be signed in to change notification settings - Fork 984
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
fix: display the value to sign as a bytes32 hex string if the data is 66 chars long #10413
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (3)
|
src/status_im/signing/core.cljs
Outdated
@@ -195,7 +195,9 @@ | |||
:signing/sign {:type (cond pinless? :pinless | |||
keycard-multiaccount? :keycard | |||
:else :password) | |||
:formatted-data (if typed? (types/json->clj data) (ethereum/hex-to-utf8 data)) | |||
:formatted-data (if typed? (types/json->clj data) (if (= 66 (count data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richard-ramos can you please extract 66
to some const and give it a name? We could also extract the whole if
form to a separate fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and please write some comments in the code why 66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
d18d5c1
to
4c26cc2
Compare
@@ -138,3 +138,13 @@ | |||
nil)] | |||
(when normalized-key | |||
(subs (sha3 normalized-key) 26)))) | |||
|
|||
(def ^:const bytes32-length 66) ; length of '0x' + 64 hex values. (a 32bytes value has 64 nibbles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but what if text message has a 66 length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be shown as a hex string. I don't think it's possible to identify if a hex value should be shown as a string or as a hexadecimal string (EIP712 fixes that with typed data). This assumes that it's more common that a 32 bytes hex string will be represented as such and not as a string value.
For comparison with Metamask, that implements this same behavior, I tried executing this instruction - Signing a string with 32 characters - and it's represented as a hex value:
web3.eth.personal.sign("ABCDEFGHABCDEFGHABCDEFGHABCDEFGH", web3.eth.defaultAccount);
However, just removing/adding one character will make the string be displayed as text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's weird, mhm dunno what to do
99% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (91)Click to expand |
100% of end-end tests have passed
Passed tests (1) |
… 66 chars long Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
4c26cc2
to
277f65b
Compare
fixes #8590
Summary
If the value to sign is a bytes32 hexadecimal string, like
0x1122334455667788112233445566778811223344556677881122334455667788
, it will be displayed as such in the Signature screen, otherwise, it will do the transformation to a UTF8 String as usual.In the issue I had mentioned before that any hexadecimal string should be displayed as such, but after reviewing this behavior in other web3 browsers, the hexadecimal string is shown only for bytes32 values.
Functional
Steps to test
status: ready