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

Replace contract with third party within the token allowance flow #18101

Merged
merged 11 commits into from
Apr 11, 2023

Conversation

filipsekulic
Copy link
Contributor

@filipsekulic filipsekulic commented Mar 10, 2023

Explanation

Screenshots/Screencaps

After

After.mov

Manual Testing Steps

  1. Go to test-dapp
  2. CREATE TOKEN
  3. APPROVE TOKENS

@filipsekulic filipsekulic self-assigned this Mar 10, 2023
@filipsekulic filipsekulic force-pushed the replace-contract-with-use-third-party branch from e87275a to a7e62c4 Compare March 10, 2023 12:30
@metamaskbot
Copy link
Collaborator

Builds ready [5c43c2f]
Page Load Metrics (1570 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint961921222311
domContentLoaded14001846154610952
load14411846157010751
domInteractive14001846154610952
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #18101 (a27542e) into develop (f92e463) will decrease coverage by 0.62%.
The diff coverage is n/a.

❗ Current head a27542e differs from pull request most recent head 9712cf2. Consider uploading reports for the commit 9712cf2 to get more accurate results

@@             Coverage Diff             @@
##           develop   #18101      +/-   ##
===========================================
- Coverage    65.15%   64.53%   -0.62%     
===========================================
  Files          936      916      -20     
  Lines        35965    35435     -530     
  Branches      9231     9124     -107     
===========================================
- Hits         23432    22867     -565     
- Misses       12533    12568      +35     

see 178 files with indirect coverage changes

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

@filipsekulic filipsekulic marked this pull request as ready for review March 10, 2023 14:57
@filipsekulic filipsekulic requested a review from a team as a code owner March 10, 2023 14:57
@metamaskbot
Copy link
Collaborator

Builds ready [f71422d]
Page Load Metrics (1585 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint972061192311
domContentLoaded12661979155914871
load12661979158515574
domInteractive12661979155914871
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

jpuri
jpuri previously approved these changes Mar 14, 2023
@bschorchit bschorchit requested a review from coreyjanssen March 20, 2023 21:32
@coreyjanssen
Copy link
Contributor

One small change in just a few spots! We need to add a hyphen when third party is used as a modifier to the word details, as seen in the screenshots here. Every other instance, there's no need for a hyphen.

Screenshot 2023-03-20 at 3 25 01 PM

Screenshot 2023-03-20 at 3 25 12 PM

In summary, please add a hyphen in the instances in the screenshots, so the copy reads third-party details. Thanks!

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

LGTM with some questions for @coreyjanssen

app/_locales/en/messages.json Outdated Show resolved Hide resolved
app/_locales/en/messages.json Outdated Show resolved Hide resolved
app/_locales/en/messages.json Show resolved Hide resolved
app/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/pages/token-allowance/index.scss Outdated Show resolved Hide resolved
@filipsekulic filipsekulic force-pushed the replace-contract-with-use-third-party branch from 0724b57 to c84d790 Compare March 21, 2023 09:19
@metamaskbot
Copy link
Collaborator

Builds ready [93b3d7b]
Page Load Metrics (1499 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89147112168
domContentLoaded1317168014738943
load1317168014998842
domInteractive1317168014738943
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

danjm
danjm previously approved these changes Mar 23, 2023
coreyjanssen
coreyjanssen previously approved these changes Mar 23, 2023
Copy link
Contributor

@coreyjanssen coreyjanssen left a comment

Choose a reason for hiding this comment

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

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

@filipsekulic filipsekulic dismissed stale reviews from coreyjanssen and danjm via a27542e March 23, 2023 17:56
@filipsekulic filipsekulic force-pushed the replace-contract-with-use-third-party branch from bafd7ea to a27542e Compare March 23, 2023 17:56
@MetaMask MetaMask deleted a comment from github-actions bot Mar 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 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.

@metamaskbot
Copy link
Collaborator

Builds ready [a27542e]
Page Load Metrics (1635 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint99147118136
domContentLoaded14091855161312861
load14261869163513464
domInteractive14091855161312861
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@coreyjanssen
Copy link
Contributor

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

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Collaborator

Builds ready [9712cf2]
Page Load Metrics (1740 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint902131212914
domContentLoaded15052274171919694
load15112274174019895
domInteractive15052274171919694
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@jpuri jpuri merged commit 54aeb1b into develop Apr 11, 2023
@jpuri jpuri deleted the replace-contract-with-use-third-party branch April 11, 2023 03:19
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace contract with a different word that applies for both EOAs and contracts
7 participants