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

feat: Create Permit Simulation #12606

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

feat: Create Permit Simulation #12606

wants to merge 32 commits into from

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Dec 9, 2024

Description

Creates Permit Simulation UI to be shown on V3 and V4 signTypedData confirmations. Signature Decoding logic to be added following this PR.

Add

  • create ButtonPill component
  • create PermitSimulation component
    • add to TypedSignV3V4 component
  • create constants/signatures.ts file
  • create hooks/useGetTokenStandardAndDetails files
  • create hooks/useTrackERC20WithoutDecimalInformation.ts
    file
  • create utils/signature.ts file
  • create utils/token.ts file

Update

  • add style param to UI/Name component
  • add style param to InfoRow Address component
  • add noBorder param to Box component
  • add TokenStandard enum
  • add canCloseOnBackdropClick param to confirmation BottomModal component
  • add skipCharacterInEnd param to shortenString common util. This mimics the same logic in extension

Designs

Simulations MVP https://www.figma.com/design/8DrinrQI4hs76Grm2F34xK/Simulations-MVP?node-id=1283-18713&node-type=canvas&t=FbRsIUjxMDxC3Get-0

Confirmations Redesign V5 https://www.figma.com/design/wcXUl6AH5KNFwKdAIv49kh/Confirmation-redesign-V5?node-id=4157-7776&node-type=canvas&m=dev

Related issues

Fixes: #12432

Manual testing steps

  1. Test Permit test-dapp button

Screenshots/Recordings

Before

After

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.

todo:
- metrics
- precision tooltip
- tests
- styles:
  - right align row values
  - hide
@digiwand digiwand requested review from a team as code owners December 9, 2024 08:20
@digiwand digiwand marked this pull request as draft December 9, 2024 08:20
Copy link
Contributor

github-actions bot commented Dec 9, 2024

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 Dec 9, 2024
@digiwand digiwand changed the title Feat: Create Permit Simulation feat: Create Permit Simulation Dec 18, 2024
@digiwand digiwand added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 7, 2025
@digiwand digiwand requested a review from a team as a code owner January 7, 2025 06:01
@digiwand digiwand added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 7, 2025
Copy link
Contributor

github-actions bot commented Jan 7, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 79a4003
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a2fd6019-42be-4009-a6d3-d309eb53d257

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 digiwand added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 7, 2025
Copy link
Contributor

github-actions bot commented Jan 7, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 8577591
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6232f15b-7bac-4ed3-8a0f-db8b1147c79e

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

Copy link
Contributor

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

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

Since the ButtonPill is not an official DS component, can you move it to components-temp?
Also can you attach the design for this to the PR

@digiwand
Copy link
Contributor Author

digiwand commented Jan 7, 2025

got it, thanks @brianacnguyen! moved 2bdb950

copied designs from the related issue to this ticket

});
};

export default styleSheet;
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be nicer to create separate PR for this component and also possibly addd storybook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review @jpuri!

yeah, I agree. In retrospect, having a tree trunk branch for this feature would have been nice since it requires much more than I anticipated initially. This PR is my first mobile feat PR, so I didn't expect to need all of these dependencies

good idea to add a storybook page. I created a separate issue ticket for this + related unit tests
#12910

}
}
/>
`;
Copy link
Contributor

@jpuri jpuri Jan 8, 2025

Choose a reason for hiding this comment

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

We are avoiding snapshot testing now in mobile re-design effort.

Copy link
Contributor Author

@digiwand digiwand Jan 8, 2025

Choose a reason for hiding this comment

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

Sgtm! I was mimicking the existing DS ButtonIcon component. It isn't too bad at this lower-level component, unlike a parent component to have the snapshot.

I created a separate issue to handle this suggestion #12910

truncatedCharLimit: TRUNCATED_NAME_CHAR_LIMIT,
truncatedStartChars: TRUNCATED_ADDRESS_START_CHARS,
truncatedEndChars: TRUNCATED_ADDRESS_END_CHARS,
skipCharacterInEnd: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: unit test coverage for new added condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests for shortenString with new param 5852d87

@@ -0,0 +1,55 @@
import { ApprovalRequest } from '@metamask/approval-controller';
Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test coverage for new util methods will be very useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 1ffe22e

@digiwand digiwand requested a review from brianacnguyen January 8, 2025 16:42
@digiwand digiwand added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 9, 2025
Copy link
Contributor

github-actions bot commented Jan 9, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5852d87
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/57d2a231-250d-444a-a615-afba71e7f846

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 digiwand requested a review from jpuri January 10, 2025 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create simulation component for permits
4 participants