Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

fallback to eth_sign if eth_signTypedData fails #1817

Merged
merged 5 commits into from
May 15, 2019

Conversation

xianny
Copy link
Contributor

@xianny xianny commented May 15, 2019

Lots of wallets/web3providers still don't support the new signing methods.

Description

Add a new method ecSignTransactionAsync that will first attempt eth_signTypedData and fall back to eth_sign if it's not available.

Testing instructions

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

xianny added 2 commits May 15, 2019 15:08
lots of wallets/web3providers still don't support the new signing methods
@xianny xianny force-pushed the fix/fallback-to-eth_sign branch from afaaa97 to 9293363 Compare May 15, 2019 19:09
@coveralls
Copy link

coveralls commented May 15, 2019

Coverage Status

Coverage decreased (-5.9%) to 78.175% when pulling ca47780 on fix/fallback-to-eth_sign into c7f474a on development.

Copy link
Contributor

@fabioberger fabioberger left a comment

Choose a reason for hiding this comment

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

Small fix required, then LGTM

if (err.message.includes('User denied message signature')) {
throw err;
}
const orderHash = transactionHashUtils.getTransactionHashHex(transaction);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const orderHash = transactionHashUtils.getTransactionHashHex(transaction);
const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);

@xianny xianny merged commit 05d3461 into development May 15, 2019
@xianny xianny deleted the fix/fallback-to-eth_sign branch May 15, 2019 22:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants