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

Cleanup approve transaction review #6213

Merged
merged 4 commits into from
Apr 29, 2023
Merged

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Apr 19, 2023

Fixes: #6159

The task was about extracting out tooltip component from ApproveTransactionReview component but I found that these components were actually not being used. Thus I delete them from code.

QA Steps:

  • The PR only removes un-used code, a check can be done to ensure that gas tooltip on confirmation pages work as before.

Screenshot 2023-04-19 at 2 18 32 PM

@jpuri jpuri requested a review from a team as a code owner April 19, 2023 08:49
@github-actions
Copy link
Contributor

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.

@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Apr 20, 2023
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.

It seem isSigningQRObject isn't passed to the ApproveTransactionReview component and so has no value. So yeah, lgtm. Great find, @jpuri

@montelaidev
Copy link

can we add some unit tests for this?

Copy link
Contributor

@plasmacorral plasmacorral left a comment

Choose a reason for hiding this comment

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

In a spot check I am I encountering an issue with spending approvals when using Keystone. After tapping approve, the button becomes disabled but the QR modal fails to open and present the composed transaction (to be scanned with Keystone).

iPhone 13 mini
iOS 15.6.1

https://recordit.co/FDiMZnGtz5

Steps to reproduce:

  1. Import SRP
  2. bind with Keystone
  3. Be on testnet or change to it (Sepolia)
  4. Open test dapp
  5. tap Create Token under Send Token (patience required here)
  6. Add token to wallet and Transfer tokens (may not be required steps)
  7. tap Approve tokens
  8. Tap Approve
  9. Note absence of QR

@jpuri jpuri marked this pull request as draft April 25, 2023 17:07
@jpuri jpuri force-pushed the approve_transaction_review_cleanup branch from 49ae910 to 8dc9271 Compare April 26, 2023 04:07
@jpuri jpuri marked this pull request as ready for review April 26, 2023 04:08
@jpuri
Copy link
Contributor Author

jpuri commented Apr 26, 2023

Hey @gantunesr , @plasmacorral : thanks a lot for helping with this. @plasmacorral : reported that this PR indeed has issue with QR scanning flow. I have updated the PR to remove QR code related cleanup. I will create a separate PR for cleaning that up.

@jpuri jpuri requested a review from plasmacorral April 26, 2023 04:10
@jpuri jpuri added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 26, 2023
@jpuri jpuri requested a review from gantunesr April 26, 2023 04:10
@seaona
Copy link
Contributor

seaona commented Apr 28, 2023

From QA changes look good @jpuri . The tooltip is working as before in the different flows (Send ETH / Send token / Contract Interaction ..) and in different networks

tooltip.mp4

PR can be merged. Whenever I get the Keystone, I can QA the other changes.

PS Thank you @plasmacorral for doing an initial QA on the QR code part !

@seaona seaona added 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. labels Apr 28, 2023
@plasmacorral
Copy link
Contributor

Just want to confirm no issues observed with Keystone approval in commit 99fa02b on the same iphone I previously tested with.

https://recordit.co/wrCPIUufPa

@jpuri jpuri merged commit 6240671 into main Apr 29, 2023
@jpuri jpuri deleted the approve_transaction_review_cleanup branch April 29, 2023 00:16
@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2023
@bschorchit bschorchit added the release-6.6.0 Issue or pull request that will be included in release 6.6.0 label May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-6.6.0 Issue or pull request that will be included in release 6.6.0 team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ApproveTransactionReview - extract function to render tooltip into a separate component.
9 participants