-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Parity Signer QR support #592
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.
i wasn't aware that UOS integration was done already. One comment about Uintarray -> string conversoin
packages/ui-qr/src/Display.tsx
Outdated
|
||
const frames = withMulti | ||
? createFrames(value) | ||
: u8aToString(value); |
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.
There might be bugs with this. if each byte is <128, then it's okay, because the utf-8 encoding will also be one byte. If not, then not sure.
Created this yesterday: kazuhikoarase/qrcode-generator#73
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.
Yes, I'm very, very worried about that. The TextDecoder/Encoder polyfills (as supplied to RN) is actually the right way, it just uses fromCharCode
- so will bring that in here, we don't have an export for it in util.
https://github.com/polkadot-js/common/blob/master/packages/util/src/string/toU8a.ts#L7
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.
I think this would work. qrcode-generator is using .charCodeAt
to do string->Uint8Array conversion, and we're using .fromCharCode
to do Uint8Array->string. They are both inverse functions from each other on 0-255
redundant work to do, but it works.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Next steps: Actually making in work in accounts and the signer. When that is done and confirmed working, the
ui-qr
package should be moved to theui
repo (Just kept here for now while things may still change)