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

[Parity Signer] Add future support for ERC-681 #1945

Merged
merged 6 commits into from
Jul 6, 2018

Conversation

maciejhirsz
Copy link
Contributor

@maciejhirsz maciejhirsz commented Jun 13, 2018

Description

Prepares the Parity Signer address scanning for the future versions that will use ERC-681 formatted strings (more context).

This change is backwards compatible, old (current) versions of Parity Signer should work without changes.

Changes

  • Updated @parity/qr-signer to 0.3.1
  • onScan will now callback with an object containing address and chainId, so made appropriate adjustments in ParitySigner.tsx.

Steps to Test

  1. Send a tx using Parity Signer.
  2. Try to unlock an account by showing a constructed QR code containing ERC-681 URL, such as ethereum:0x5555555555555555555555555555555555555555@42 (chainId is being parsed, but we throw it away right now)

@maciejhirsz
Copy link
Contributor Author

Note: I got tscheck errors when trying to push, but the same errors were thrown on develop branch without changes

@maciejhirsz maciejhirsz changed the title Add future support for ERC-681 [Parity Signer] Add future support for ERC-681 Jun 13, 2018
@HenryNguyen5
Copy link

If you're using npm its known to throw ts errors due to version mismatch, yarn should work fine

@coveralls
Copy link

coveralls commented Jun 13, 2018

Coverage Status

Coverage remained the same at 56.28% when pulling b8fe3bd on maciejhirsz:parity-signer-erc681 into fbe792a on MyCryptoHQ:develop.

@@ -94,7 +94,7 @@ class QrSignerModal extends React.Component<Props, State> {
return;
}

this.props.finalizeSignature(signature);
this.props.finalizeSignature('0x' + signature);
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO (for myself): circle back and provide utility function for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

addHexPrefix does this from ethereumjs-util

Copy link
Contributor

Choose a reason for hiding this comment

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

@maciejhirsz You cool with making this small tweak? No reason not to merge after imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

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.

5 participants