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

Use personal_sign when injected web3provider is TrustWallet #1542

Closed
wants to merge 1 commit into from

Conversation

alfetopito
Copy link

Summary

Using personal_sign instead of eth_sign for TrustWallet.

This sort of a follow up to my comment on a similar PR #1285 (comment)

TrustWallet's eth_sign method does not properly encode the data.
With this workaround I'm able to perform the signing as expected.

The flag isTrust is set by TrustWallet's web3 provider https://github.com/trustwallet/trust-web3-provider/blob/93395938bb6838e35e3e358bcf952124131186fa/README.md#L15 similar to isMetaMask and isStatus flags.

According to MM docs, personal_sign is preferred instead of eth_sign.

And now that I went through TrustWallet's web3 provider code I found out the reasoning for different choice of method: trustwallet/trust-web3-provider#102 (comment)

Although, to be honest, I'm not 100% clear on that.

Testing

Well, I would have published a RC npm package with these changes under my NPM account, or at a minimum publish it using yalc locally, but setting up ethers.js is a bit tricky.
So I went with the good old "copy generated files to destination node_modules folder". And it worked as expected :)

If you insist, I can work on a minimum code sample.

@alfetopito
Copy link
Author

FYI, we kind of need this change sooner rather than later, so I went ahead and published the providers package under my account https://www.npmjs.com/package/ethers_providers_alfetopito

I made a mistake and screwed the package name, but it's working and makes it explicit for us that this is just a temporary fork.
When this change is merged and released with ethersjs I'll clean that mess up :)

@ricmoo
Copy link
Member

ricmoo commented May 6, 2021

I would live to make this change, but am waiting for someone to provide the research outlined here:

#1285 (comment)

I’ll open a new issue to track this.

@ricmoo
Copy link
Member

ricmoo commented May 6, 2021

Please see #1544.

Add any environments you have access to. :)

hanzelnut pushed a commit to hanzelnut/ethers.js that referenced this pull request Aug 10, 2021
@rhlsthrm
Copy link

rhlsthrm commented Sep 3, 2021

Any update on this? We need this change as well.

@ricmoo
Copy link
Member

ricmoo commented Sep 3, 2021

I’ll be making the switch on the next minor version bump.

@ricmoo ricmoo added minor-bump Planned for the next minor version bump. on-deck This Enhancement or Bug is currently being worked on. labels Sep 3, 2021
@rhlsthrm
Copy link

rhlsthrm commented Sep 3, 2021

Any timeline for this?

@ricmoo
Copy link
Member

ricmoo commented Sep 3, 2021

Not yet, I’ve just finished off a few major tasks, and have 2 presentations to prepare for. Maybe within 2 weeks?

@rhlsthrm
Copy link

rhlsthrm commented Sep 3, 2021

Thanks @ricmoo. Will be watching this and implement a workaround in the meantime.

@rhlsthrm
Copy link

Ping @ricmoo any update here?

ricmoo added a commit that referenced this pull request Oct 16, 2021
…cSigner; added _legacySignMessage for legacy support (#1542, #1840).
@ricmoo
Copy link
Member

ricmoo commented Oct 16, 2021

Useful info: WalletConnect/walletconnect-monorepo#1395

Starting in v5, the JsonRpcSigner will use personal_sign, but a new method provider._legacySignMessage has been added for compatibility for those who need it. This legacy method will likely go away entirely in v6 though.

@ricmoo
Copy link
Member

ricmoo commented Oct 20, 2021

This has been updated in 5.5.0.

For those who require the old behaviour, the JsonRpcSigner has a _legacySignMessage method which mimics the old behaviour; but not that this is functionality most clients no longer support, or if they support the method, it performs prefixing breaking legacy anyways. In v6, this method will go away.

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. minor-bump Planned for the next minor version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants