feat: Redesigned setApprovalForAll confirmations #27061
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
pedronfigueiredo
force-pushed
the
pnf/2937
branch
4 times, most recently
from
fa7fa67
to
8205e7b
Compare
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
94d7fc7
to
93dbee0
Compare
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
93dbee0
to
1244155
Compare
Builds ready [1244155]
Page Load Metrics (1767 ± 73 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
1244155
to
4a0bd3a
Compare
Builds ready [4a0bd3a]
Page Load Metrics (1568 ± 68 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #27061 +/- ##
===========================================
- Coverage 70.09% 70.01% -0.08%
===========================================
Files 1438 1447 +9
Lines 49971 50218 +247
Branches 13989 14048 +59
===========================================
+ Hits 35026 35157 +131
- Misses 14945 15061 +116 ☔ View full report in Codecov by Sentry. |
OGPoyraz
previously approved these changes
Sep 13, 2024
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
4a0bd3a
to
1fd42fe
Compare
digiwand
reviewed
Sep 13, 2024
Comment on lines
46
to
62
| await withFixtures( | ||
| { | ||
| dapp: true, | ||
| fixtures: new FixtureBuilder() | ||
| .withPermissionControllerConnectedToTestDapp() | ||
| .withPreferencesController({ | ||
| preferences: { | ||
| redesignedConfirmationsEnabled: true, | ||
| isRedesignedConfirmationsDeveloperEnabled: true, | ||
| }, | ||
| }) | ||
| .build(), | ||
| ganacheOptions: defaultGanacheOptionsForType2Transactions, | ||
| smartContract, | ||
| testSpecificMock: mocks, | ||
| title: this.test?.fullTitle(), | ||
| }, |
There was a problem hiding this comment.
[comment]
we have a helper method withRedesignConfirmationFixtures which we could update to support more options. The more I think about it, we could do what we did in the last PR and create a method to generate the fixture options object with these options instead.
digiwand
reviewed
Sep 13, 2024
| @@ -16,7 +16,11 @@ import { | |||
| RecipientRow, | |||
| } from '../../shared/transaction-details/transaction-details'; | |||
|
|
|||
| const Spender = () => { | |||
| const Spender = ({ | |||
| isSetApprovalForAll, | |||
There was a problem hiding this comment.
[nit] we could set a default value to fix type to boolean
Suggested change
| isSetApprovalForAll, | |
| isSetApprovalForAll = false, |
digiwand
reviewed
Sep 13, 2024
| @@ -44,14 +51,18 @@ const Spender = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| export const ApproveDetails = () => { | |||
| export const ApproveDetails = ({ | |||
| isSetApprovalForAll, | |||
There was a problem hiding this comment.
[nit] we could set a default value to fix type to boolean
Suggested change
| isSetApprovalForAll, | |
| isSetApprovalForAll = false, |
digiwand
previously approved these changes
Sep 13, 2024
Builds ready [1fd42fe]
Page Load Metrics (1783 ± 127 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
matthewwalsh0
previously approved these changes
Sep 14, 2024
test/e2e/tests/confirmations/transactions/erc1155-set-approval-for-all-redesign.spec.ts
Outdated
Show resolved
Hide resolved
...onfirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.tsx
Outdated
Show resolved
Hide resolved
pedronfigueiredo
dismissed stale reviews from matthewwalsh0 and digiwand
via
1ed1d28
OGPoyraz
previously approved these changes
Sep 23, 2024
|
2 tests suites failing which requires updates: minor lint fix SonarCloud is flagging coverage < 80% |
pedronfigueiredo
dismissed stale reviews from OGPoyraz and matthewwalsh0
via
6d267b0
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
3bf14ef
to
6d267b0
Compare
pedronfigueiredo
force-pushed
the
pnf/2937
branch
from
6d267b0
to
717c4ff
Compare
|
OGPoyraz
approved these changes
Sep 24, 2024
matthewwalsh0
approved these changes
Sep 24, 2024
Builds ready [717c4ff]
Page Load Metrics (1759 ± 103 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
release-12.6.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implement setApprovalForAll redesigned confirmation for ERC721 and ERC1155 tokens. Includes e2e tests that use the Page Object Model, and refactors withRedesignedConfirmations to support EIP1559 transactions.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2937
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist