-
Notifications
You must be signed in to change notification settings - Fork 360
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 fail amino sign verify with special chars #1373
Conversation
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.
Thank you for this PR. Since we only care about JSON strings and not the whole JSON document, I think it is cleaner and safer to use the replacer argument in JSON.stringify.
Here is some example how this can be used to manipulate the inner strings as part of the stringification: https://gist.github.com/Tanapruk/4ab1505ead2491229235c886b6fbfaac
hey @loin3, thanks for doing this :) eagerly awaiting this over at DAO DAO because our users run into this bug all the time |
Sorry, I mixed up issue and PR here. I was referring to
|
I agree that this implementation should be changed to something tidy way. because modifying the byte code directly is very ugly. here is the reason why I should modify byte code directly. |
Oh, I see. This makes sense, thank you. In this case the current implementation makes sense. I'll have another look and throw a bunch of low level unit tests on the implementation to better understand it. Thanks a lot! |
Oh damn, sorry, wrong PR. I did not aim to merge this yet. Reverted the merge. I'll look into this ASAP. |
I used this work in #1388. There I migrated from binary to string based replacement and increased low level test coverage. Reviews appreciated. |
fix fail amino sign verify with special chars
this PR is for #1367
I found that there is a problem with escaping special chars in keplr-wallet. If you follow this method, the value is still escaped when you check memo of tx in mintscan. Because cosmjs sends memo through protobuf, the value needs to be escaped only when signed with amino. So escaping is done only on values encoded for amino signatures. This way, even if you look up the tx, the memo will not be escaped.