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

[FIX] Wallet connect signed typed and eth sign throwing error back to the dapp #5103

Merged
merged 6 commits into from
Nov 23, 2022

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Oct 5, 2022

Description
Wallet connect default sign typed data function we redirect it to v3 and if the dapp send a message with array structured we do not support it.
We also were not returning back the eth sign function if it went wrong back to the dapp.

Proposed Solution
When a user signs a message that is only supported with v4 with the default sign method of wallet connect, we throw the error back to the dapp and say to our user that the sign failed.
We also send back to the dapp the error if eth sign fails.

Screenshots/Recordings
https://recordit.co/mH3Es44m1J

Test Cases
Case1:

  • Clone metamask test dapp from this github https://github.com/tommasini/test-dapp
  • switch to branch wc-support
  • Open the project and on terminal type yarn setup and then yarn start
  • Tested all the functions to test with wallet connect at the bottom of test dapp

Case2:

Code impact
Low

Issue

Progresses ##4441, #3887, #3580

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@tommasini tommasini requested a review from a team as a code owner October 5, 2022 22:58
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Oct 5, 2022
@kevinwo
Copy link

kevinwo commented Oct 13, 2022

While it looks like this PR resolves the silent failure when attempting to sign with V4, will there be a follow-on PR then that upgrades the WalletConnect library to >= 1.8.0 and support V4 signing?

@tommasini
Copy link
Contributor Author

Hello @kevinwo , thanks for testing the PR!

I'm not sure what you are asking but you can do a sign request V4 if you stablish connection with wallet connect and send a custom request with the method eth_signTypedData_v4, just the default sign method of wallet connect will be redirected to the V3

@kevinwo
Copy link

kevinwo commented Oct 13, 2022

you can do a sign request V4 if you stablish connection with wallet connect and send a custom request with the method eth_signTypedData_v4

@tommasini Ahh yes, that makes sense. Thank you!

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of small comments

app/core/RPCMethods/RPCMethodMiddleware.ts Outdated Show resolved Hide resolved
app/components/UI/TypedSign/index.js Outdated Show resolved Hide resolved
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@gantunesr gantunesr added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Nov 3, 2022
@andreahaku andreahaku added release-5.12.0 Priority - High Task with high priority labels Nov 21, 2022
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@tommasini swinging this back your way!

Issue 1
On your clone repo when I tried connecting to wallet connect by scanning the QR code nothing happens. I am not prompted with the connect modal. Furthermore, when I attempt to scan the QR code again, I get prompted that the QR was already scanned. See recording.

Issue 2
When I tried signing WC v4 I get Error: TypedMessageManager: Message not found for id: undefined.

see recording.

To reproduce:
Connect to wallet connect and try signing WC sign Typed Data V4

Issue 3
Eth sign is broken on your repo. I get Error: eth_sign requires 32 byte message hash

To reproduce:
Connect to the test dapp on your repo
Tap Eth Personal Sign
Notice the error

See recording

Keep in mind, the regular test dapp is working as expected!

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Nov 23, 2022
@tommasini
Copy link
Contributor Author

tommasini commented Nov 23, 2022

Thank you for testing @cortisiko!

Issue 1:
I'm trying to reproduce but every time I press connect again after I disconnect, everything went normal
Issue 2:
I'm not being able to reproduce it, and I tried via in-app browser, desktop and chrome mobile, everything went as expected
Issue 3:
It's the expected behaviour, it's to demonstrate that we are sending the error back to the dapp (we can confirm seeing the response on the test dapp)

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Nov 23, 2022
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@tommasini thanks for the update. This PR is QA passed. Things I tested:

  • signing with WC on a couple dapps such as opensea and rariable
  • signing on our test dapp
  • signing with WC on your (@tommasini) test dapp
  • signing WC with keystone
  • signing regular dapps with keystone
  • WC transactions

@cortisiko cortisiko merged commit 1c0ebb4 into main Nov 23, 2022
@cortisiko cortisiko deleted the fix/wc-eth-sign-sign-typed-data branch November 23, 2022 20:53
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority - High Task with high priority QA Passed A successful QA run through has been done release-5.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants