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

Allow specific origin to direct to second page of token allowance flow #18395

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Mar 31, 2023

Explanation

Bridging functionality on the portfolio dapp is non-optimal because users are unknowingly customizing their token allowances to error prone amounts. This PR mitigates the problem by taking those users directly to second screen of the token allowance flow, thereby lowering the chance that they inadvertently customize the pDapps suggested allowance amount.

Screenshots/Screencaps

Peek.2023-03-31.00-35.mp4

Manual Testing Steps

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@danjm danjm requested a review from a team as a code owner March 31, 2023 03:12
@danjm danjm requested a review from adonesky1 March 31, 2023 03:12
@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.

ui/pages/token-allowance/token-allowance.js Outdated Show resolved Hide resolved
ui/pages/token-allowance/token-allowance.js Outdated Show resolved Hide resolved
ui/pages/token-allowance/token-allowance.js Outdated Show resolved Hide resolved
@rekmarks rekmarks dismissed their stale review March 31, 2023 04:16

My concerns are resolved but I shouldn't be the one to approve.

@metamaskbot
Copy link
Collaborator

Builds ready [0511fcd]
Page Load Metrics (1595 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint94130107115
domContentLoaded14371991157713063
load14371991159513062
domInteractive14371991157713063
Bundle size diffs
  • background: 0 bytes
  • ui: 112 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #18395 (0511fcd) into develop (ed3cc40) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff            @@
##           develop   #18395   +/-   ##
========================================
  Coverage    64.69%   64.69%           
========================================
  Files          925      925           
  Lines        35584    35590    +6     
  Branches      9136     9138    +2     
========================================
+ Hits         23018    23023    +5     
- Misses       12566    12567    +1     
Impacted Files Coverage Δ
ui/pages/token-allowance/token-allowance.js 64.23% <85.71%> (+0.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jpuri
Copy link
Contributor

jpuri commented Mar 31, 2023

@danjm : the change looks good to me. I will create a followup ticket for confirmations team to add unit test cases for this.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! But it we should add an e2e test case for this as well

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

I looked for ways to spoof that hostname but none still work, so this seems reasonable.

@danjm danjm merged commit 4121008 into develop Mar 31, 2023
@danjm danjm deleted the allow-pdapp-tkn-allowance branch March 31, 2023 14:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2023
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.

7 participants