feat: Implement unstaking confirmation #13921

pedronfigueiredo · 2025-03-07T15:07:38Z

Description

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4379

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2025-03-07T15:07:53Z

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.

github-actions · 2025-03-07T15:08:17Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 23f65b0
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b42e7939-a23a-4794-a8ef-51aa286ed641

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

digiwand · 2025-03-08T05:04:00Z

app/components/Views/confirmations/components/Confirm/AdvancedDetails/AdvancedDetails.tsx

+
+

Suggested change

digiwand · 2025-03-08T05:08:29Z

app/components/Views/confirmations/Confirm/Confirm.test.tsx

+        }
+      }),
+    });
+    expect(getByText('Withdrawal time')).toBeDefined();


To confirm, is this is the confirmed copy? Figma is using "Unstaking time"

I confirmed with Saya that we want to keep withdrawal time, the designs will be updated

OGPoyraz · 2025-03-11T08:59:44Z

app/components/Views/confirmations/Confirm/Confirm.test.tsx

+            AccountsController: {
+              internalAccounts: {
+                accounts: {
+                  '0x0000000000000000000000000000000000000000': {
+                    address: '0x0000000000000000000000000000000000000000',
+                  },
+                },
+                selectedAccount: '0x0000000000000000000000000000000000000000',
+              },
+            },


Does it make sense to put this AccountsController state into stakingWithdrawalConfirmationState? It looks a needed state almost all tests.

OGPoyraz · 2025-03-11T09:00:45Z

app/components/Views/confirmations/Confirm/Confirm.test.tsx

 // eslint-disable-next-line import/no-namespace
 import * as ConfirmationRedesignEnabled from '../hooks/useConfirmationRedesignEnabled';

+import { merge } from 'lodash';


Minor, can we make package imports on top of file

OGPoyraz · 2025-03-11T09:00:50Z

app/components/Views/confirmations/Confirm/Confirm.test.tsx

 // eslint-disable-next-line import/no-namespace
 import * as ConfirmationRedesignEnabled from '../hooks/useConfirmationRedesignEnabled';

+import { merge } from 'lodash';


Minor, can we make package imports on top of file

OGPoyraz · 2025-03-11T09:08:04Z

app/components/Views/confirmations/components/Confirm/AdvancedDetails/AdvancedDetails.tsx

 import styleSheet from './AdvancedDetails.styles';

 const AdvancedDetails = () => {
  const { styles } = useStyles(styleSheet, {});
-  const transactionMeta = useTransactionMetadataRequest();

  return (
    <View style={styles.container}>
      <InfoSectionAccordion header={strings('stake.advanced_details')}>


If we think of this component as a generic one - does it make sense to change stake.advanced_details to confirmations.advanced_details or something similar?

I am not sure if we will use this component as generic to more confirmation types. InfoSectionAccordion can be leveraged for other advanced details, but the internals will need to be changed significantly if we wanted to reuse this specific component.

If we don't think this as generic component then this is just an extra file hop in the StakingDeposit file. If nothing on this component will be re-used - then would it make sense do

<InfoSectionAccordion header={strings('stake.advanced_details')}> <ContractInteractionDetails variant={ContractInteractionDetailsVariant.StakingDeposit} /> </InfoSectionAccordion>

instead of

<AdvancedDetails />

in StakingDeposit?

Because similar thing has done in StakingWithdrawal already

fair enough, I'll remove the advanced details file 👍

OGPoyraz · 2025-03-11T09:10:46Z

app/components/Views/confirmations/components/Confirm/AdvancedDetails/AdvancedDetails.tsx

-            <Text>{strings('stake.ethereum_mainnet')}</Text>
-          </View>
-        </InfoRow>
+        <ContractInteractionDetails variant={ContractInteractionDetailsVariant.StakingDeposit} />


As similar note above, what do we think of deriving variant in ContractInteractionDetails? Because if this is a generic AdvancedDetails component then it shouldn't rely on any variant passed by this component.

We could also fix these similar queries by introducing ConfirmationContext as we do in extension. I will probably introduce ConfirmationContext in #13919

It's a good thought, I will change to using the transaction type internally in the contract interaction details instead of the variant.

Minor, on related note if ContractInteractionDetails is just purpose of serving those details only for staking operations - maybe we could rename this into StakingContractInteractionDetails. Since we are planning to give code ownership to them it would be more clear if we rename like that. We could do this change later - it's not urgent, just flagging here.

github-actions · 2025-03-11T10:37:36Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: da36f6c
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e3bff41f-ca15-4e42-9f38-6cbde8bd9fdb

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

sonarqubecloud · 2025-03-12T10:50:22Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Mar 7, 2025

pedronfigueiredo self-assigned this Mar 7, 2025

pedronfigueiredo requested a review from a team as a code owner March 7, 2025 15:07

pedronfigueiredo added the Run Smoke E2E Triggers smoke e2e on Bitrise label Mar 7, 2025

pedronfigueiredo force-pushed the pnf/unstaking-confirmation branch 2 times, most recently from b9938cc to 657a0c7 Compare March 7, 2025 16:52

pedronfigueiredo requested a review from a team as a code owner March 7, 2025 16:52

pedronfigueiredo force-pushed the pnf/unstaking-confirmation branch from 657a0c7 to bc92294 Compare March 7, 2025 17:07

digiwand reviewed Mar 8, 2025

View reviewed changes

digiwand previously approved these changes Mar 8, 2025

View reviewed changes

OGPoyraz reviewed Mar 11, 2025

View reviewed changes

pedronfigueiredo dismissed digiwand’s stale review via da36f6c March 11, 2025 10:36

pedronfigueiredo added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Mar 11, 2025

pedronfigueiredo added 3 commits March 12, 2025 10:37

feat: Implement unstaking confirmation

b69c12e

feat: Implement unstaking confirmation

1e85a70

feat: Implement unstaking confirmation

16d6d56

pedronfigueiredo force-pushed the pnf/unstaking-confirmation branch from 9482b67 to 16d6d56 Compare March 12, 2025 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement unstaking confirmation #13921

feat: Implement unstaking confirmation #13921

pedronfigueiredo commented Mar 7, 2025

github-actions bot commented Mar 7, 2025

github-actions bot commented Mar 7, 2025 •

edited by metamaskbot

Loading

digiwand Mar 8, 2025

digiwand Mar 8, 2025

pedronfigueiredo Mar 11, 2025

OGPoyraz Mar 11, 2025

OGPoyraz Mar 11, 2025

OGPoyraz Mar 11, 2025

OGPoyraz Mar 11, 2025

pedronfigueiredo Mar 11, 2025

OGPoyraz Mar 11, 2025 •

edited

Loading

pedronfigueiredo Mar 11, 2025

OGPoyraz Mar 11, 2025

pedronfigueiredo Mar 11, 2025

OGPoyraz Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025 •

edited by metamaskbot

Loading

sonarqubecloud bot commented Mar 12, 2025

feat: Implement unstaking confirmation #13921

Are you sure you want to change the base?

feat: Implement unstaking confirmation #13921

Conversation

pedronfigueiredo commented Mar 7, 2025

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Mar 7, 2025

github-actions bot commented Mar 7, 2025 • edited by metamaskbot Loading

Bitrise

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGPoyraz Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGPoyraz Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2025 • edited by metamaskbot Loading

Bitrise

sonarqubecloud bot commented Mar 12, 2025

Quality Gate passed

github-actions bot commented Mar 7, 2025 •

edited by metamaskbot

Loading

OGPoyraz Mar 11, 2025 •

edited

Loading

OGPoyraz Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025 •

edited by metamaskbot

Loading