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

Improve signature request message #5711

Merged
merged 8 commits into from
Feb 27, 2023
Merged

Improve signature request message #5711

merged 8 commits into from
Feb 27, 2023

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Feb 6, 2023

@jpuri jpuri requested a review from a team as a code owner February 6, 2023 16:35
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

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.

@jpuri
Copy link
Contributor Author

jpuri commented Feb 6, 2023

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

Great PR. Do you think we should also add this to MessageSign as well?

@jpuri
Copy link
Contributor Author

jpuri commented Feb 7, 2023

Great PR. Do you think we should also add this to MessageSign as well?

Message sign is encoded data, its is not needed there.

Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

Lgtm

@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board labels Feb 8, 2023
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Nice 🙌

@seaona
Copy link
Contributor

seaona commented Feb 17, 2023

I've QAd the branch and when I click on Personal Sign I am seeing a crashing error saying str.replaceAll is not a function. See below:

mobile-personal-sign-regex.mp4

@seaona seaona 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. Mobile QA board labels Feb 17, 2023
@jpuri
Copy link
Contributor Author

jpuri commented Feb 20, 2023

@seaona : I found that it was crashing in android, PR is updated with fix.

@jpuri jpuri added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 20, 2023
@seaona
Copy link
Contributor

seaona commented Feb 22, 2023

The error is fixed now. Thank you @jpuri

I've realized that when we try to Verify the Signature, the verification fails and the address is not recovered. This happens only for that specific case that the original issue was reporting. For the rest of the cases the Verify Signature works fine.

I didn't realize this is also happening with the Improve signature on Extension. It is a rare edge case but we might want to resolve it correctly too. In that case I can open a new issue for Extension.

The error message is:

Failed to verify signer when comparing 0x9c90317969c3a97587acb0fee81e88778fe6ee95 to 0x1c53dc20d1e36ed8359250de626acae36bd28a29
personal-sign-mobile-verify.mp4

image

@seaona seaona added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 24, 2023
@seaona
Copy link
Contributor

seaona commented Feb 27, 2023

The above issue it's been discussed internally if needs a fix or not. Not exclusive to this PR.

@seaona seaona merged commit 3914922 into main Feb 27, 2023
@seaona seaona deleted the request_sign_improvement branch February 27, 2023 14:24
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants