Skip to content

User to be able to edit approval amount in simulation section of batched confirmation #15572

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

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented May 23, 2025

Description

User to be able to edit approval amount in simulation section of batched confirmation

Related issues

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

Manual testing steps

  1. Enable 7702 locally
  2. Submit a batched approval
  3. Edit approval amount in simulation section, ensure it works as expected

Screenshots/Recordings

Screen.Recording.2025-05-23.at.6.08.34.PM.mov

Pre-merge author checklist

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.

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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label May 23, 2025
@jpuri jpuri marked this pull request as ready for review May 23, 2025 12:40
@jpuri jpuri requested a review from a team as a code owner May 23, 2025 12:40
@jpuri jpuri enabled auto-merge May 28, 2025 11:05
@jpuri jpuri added the Run Smoke E2E Requires smoke E2E testing label May 29, 2025
Copy link
Contributor

github-actions bot commented May 29, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: f1548dd
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/dd8d29b2-6bb8-486b-8177-d9e8ece9e4c3

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

@bschorchit bschorchit requested review from OGPoyraz and digiwand May 29, 2025 14:19
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

hey @jpuri, the new feature is looking great! a couple of requests and suggestions above

@jpuri jpuri requested a review from digiwand May 30, 2025 05:03
Comment on lines +69 to +70
// eslint-disable-next-line @typescript-eslint/no-explicit-any
balance: '0x5' as any,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line @typescript-eslint/no-explicit-any
balance: '0x5' as any,
balance: '0x5' as Hex,

Does Hex works here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that it does not work, it actually needs an instance of BN but it is not in dependency in project so I am not able to use it like new BN('0x5') or '0x5' as BN

Comment on lines +76 to +87
<Text
style={styles.text}
color={TextColor.Alternative}
variant={TextVariant.BodyMD}
>
{editTexts?.description}
</Text>
<TextField
style={styles.input}
value={updatedAmount}
onChange={(evt) => setUpdatedAmount(evt.nativeEvent.text)}
/>
Copy link
Member

Choose a reason for hiding this comment

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

What do we think of re-using TextFieldWithLabel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case input does not really have a label, it is description in modal.
Screenshot 2025-05-30 at 3 37 14 PM

Copy link
Member

Choose a reason for hiding this comment

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

Just for curiosity - have we ever tried this for approval that has NFTs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yep, I tried approvals of ERC20 only, these changes need to be tested.

Comment on lines 7 to 10
const SIGNATURE_LEGACY = 'function approve(address,uint256)';
const SIGNATURE_PERMIT2 = 'function approve(address,address,uint160,uint48)';
const SIGNATURE_INCREASE_ALLOWANCE =
'function increaseAllowance(address,uint256)';
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to put them into app/components/Views/confirmations/constants/approvals.ts? Also maybe INCREASE_ALLOWANCE_SIGNATURE maybe a better naming here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the constants, I would prefer to keep name same as extension.

Comment on lines +34 to +36
args?._value ?? // ERC-20 - approve
args?.increment ?? // Fiat Token V2 - increaseAllowance
args?.amount; // Permit2 - approve
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for noting those ❤️

@jpuri jpuri requested a review from OGPoyraz May 30, 2025 10:38
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run Smoke E2E Requires smoke E2E testing team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants