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

QR support for UOS #273

Closed
wants to merge 136 commits into from
Closed

QR support for UOS #273

wants to merge 136 commits into from

Conversation

pmespresso
Copy link
Contributor

@pmespresso pmespresso commented Jul 22, 2019

  • - QrView: would not change. However in its usage for displaying addresses, it is expected to be in UOS introduction format, with extra information at the end (either a chainId or a genesisHash)
  • - QrScanner:
    • - convert rawBytes from RNNative to U8a (Unit Tested)
    • - strip padding, length, frame info (Unit Tested)
    • - parse address, crypto (if substrate), payload, data as per UOS specification (Unit Tested)
    • - encode address from bytes to ss58 (Unit Tested)
    • - merging in from the identicon PR thank you @Tbaut for this one!
    • - decode SCALE payload
      • - Cold Signer SHOULD decode the transaction details from the SCALE encoding and display them to the user for verification before signing. (Unit Tested)
        • payload MUST be the SCALE encoding of the tuple of transaction items (nonce, call, era_description, era_header).
        • payload_hash MUST be the Blake2s 32-byte hash of the SCALE encoding of the tuple of transaction items (nonce, call, era_description, era_header).
        • immortal_payload MUST be the SCALE encoding of the tuple of transaction items (nonce, call).
      • - Cold Signer SHOULD attempt to decode the message as UTF-8 encoded human readable string by whatever heuristics it finds suitable (Unit Tested) and display it to the user for verification before signing.
      • - Cold Signer SHOULD warn the user that signing a hash is inherently insecure, in the cash of type 01.
      • - Cold Signer SHOULD (at the user's discretion) sign the message, immortal_payload, or payload if payload is of length 256 bytes or fewer.
      • - If payload is longer than 256 bytes, then it SHOULD instead sign the Blake2s hash of payload.
      • - Cold Signer SHOULD display all account id values in SS58Check encoding. [Substrate accounts management #293 - It should be stored in AccountStore as such]
  • - ScannerStore:
    • - setDataToSign with appropriate native method based on network type, i.e. was crypto field defined (substrate) or not (eth) (Unit Tested)
    • - setTxToSign with appropriate hashing method based on network type. keccak for Ethereum, sr25519 or ed25519 for Substrate.

@Tbaut Tbaut added the A1-onice label Jul 23, 2019
@maciejhirsz
Copy link
Contributor

I don't think this will work for you, as @parity/qr-signer depends on web APIs for the webcam.

@Tbaut Tbaut removed the A1-onice label Jul 23, 2019
src/components/QrView.js Outdated Show resolved Hide resolved
@pmespresso
Copy link
Contributor Author

pmespresso commented Aug 19, 2019

### Sign Message ### Sign Hash ### Sign Extrinsic Payload
ezgif-2-4421c6d52e6c ezgif-2-96f04dc4aa26 ezgif-1-1ebae92564c0

@pmespresso
Copy link
Contributor Author

pmespresso commented Aug 21, 2019

Multipart will be a separate PR, though the logic is there I have no way to test them atm

@pmespresso pmespresso marked this pull request as ready for review August 21, 2019 22:26
@pmespresso pmespresso changed the title feat: wip for adding QR support for UOS QR support for UOS Aug 21, 2019
@pmespresso pmespresso mentioned this pull request Aug 21, 2019
6 tasks
@hanwencheng
Copy link
Contributor

Could you please rebase the project on master so the commits on master will not be shown here? Then it will be clear to check the differences.

@pmespresso
Copy link
Contributor Author

yes, doing this now

@pmespresso pmespresso mentioned this pull request Aug 22, 2019
17 tasks
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one issue, some other grumbles.

*/

export default function keyExtract (suri) {
const RE_CAPTURE = /^((\/\/?[^/]+)*)(\/\/\/(.*))?$/;
const RE_CAPTURE = /^(\w+( \w+)*)?((\/\/?[^/]+)*)(\/\/\/(.*))?$/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: you can silence capturing groups with ?: inside parenthesis. With regex /^(\w+(?: \w+)*)?((?:\/\/?[^/]+)*)(\/\/\/(?:.*))?$/ you can then simply do [, phrase = '', derivePath = '', password = ''].

return this.state.accounts.get(accountId(account)) || empty(account);
}

<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging artifact

let phrase, derivePath, password = '';

if (matches) {
[, phrase = '', , derivePath = '', , , password = ''] = matches;
Copy link
Contributor

@maciejhirsz maciejhirsz Aug 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

password here includes the /// (if it exists), but every time you construct a suri you also add the slashes. This means the slashes will be included in the suri, which will lead to //////password. Edit: my mistake, slashes are stripped.

I'd recommend having two functions here: one to convert a suri string to an object, and another to convert the object to a suri, next to each other so the logic that does the conversion is contained next to each other in a single file, is easy to audit. Avoid constructing the suri string in any other places for security. This is pretty critical, people can lose money if their derivations don't work as expected.

@pmespresso
Copy link
Contributor Author

moved to #325

@pmespresso pmespresso closed this Aug 23, 2019
@Tbaut Tbaut deleted the yj-ar branch September 17, 2019 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants