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

[Signature Redesign] fixes to PermitBatch and PermitBatchTransferFrom simulation #26592

Closed
9 tasks
bschorchit opened this issue Aug 21, 2024 · 1 comment · Fixed by #26684
Closed
9 tasks
Assignees
Labels
regression-RC-12.3.0 Regression bug that was found in release candidate (RC) for release 12.3.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 release-blocker This bug is blocking the next release team-confirmations Push issues to confirmations team type-bug

Comments

@bschorchit
Copy link

bschorchit commented Aug 21, 2024

What is this about?

Within the estimated changes section, we should:

  • display the token address for each token in the payload instead of verifyingContract address
  • display the amount and fiat conversion information for each token in the payload

Scenario

No response

Design

Current UI:
Screenshot 2024-08-21 at 14 47 06

Desired UI:

Screenshot 2024-08-15 at 16 42 20

Figma file

Technical Details

  • See type definition here.
  • See random example transaction including signature here.

Example signature payload:

{
    "types": {
        "PermitBatch": [
            {
                "name": "details",
                "type": "PermitDetails[]"
            },
            {
                "name": "spender",
                "type": "address"
            },
            {
                "name": "sigDeadline",
                "type": "uint256"
            }
        ],
        "PermitDetails": [
            {
                "name": "token",
                "type": "address"
            },
            {
                "name": "amount",
                "type": "uint160"
            },
            {
                "name": "expiration",
                "type": "uint48"
            },
            {
                "name": "nonce",
                "type": "uint48"
            }
        ],
        "EIP712Domain": [
            {
                "name": "name",
                "type": "string"
            },
            {
                "name": "chainId",
                "type": "uint256"
            },
            {
                "name": "verifyingContract",
                "type": "address"
            }
        ]
    },
    "domain": {
        "name": "Permit2",
        "chainId": "1",
        "verifyingContract": "0x000000000022d473030f116ddee9f6b43ac78ba3"
    },
    "primaryType": "PermitBatch",
    "message": {
        "details": [{
            "token": "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48",
            "amount": "1461501637330902918203684832716283019655932542975",
            "expiration": "1722887542",
            "nonce": "5"
        }, {
            "token": "0xb0b86991c6218b36c1d19d4a2e9eb0ce3606eb48",
            "amount": "2461501637330902918203684832716283019655932542975",
            "expiration": "1722887642",
            "nonce": "6"
        }],
        "spender": "0x3fc91a3afd70395cd496c647d5a6cc9d4b2b7fad",
        "sigDeadline": "1720297342"
    }
}

Threat Modeling Framework

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

No response

@bschorchit bschorchit added release-blocker This bug is blocking the next release team-confirmations Push issues to confirmations team release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Aug 21, 2024
@digiwand digiwand changed the title [Signature Redesign] fixes to PermitBatch [Signature Redesign] fixes to PermitBatch simulation Aug 22, 2024
@digiwand digiwand self-assigned this Aug 23, 2024
@digiwand
Copy link
Contributor

Solution is combined with PermitTransferFrom and PermitBatchTransferFrom support

@digiwand digiwand changed the title [Signature Redesign] fixes to PermitBatch simulation [Signature Redesign] fixes to PermitBatch and PermitBatchTransferFrom simulation Aug 31, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Sep 3, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Sep 4, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 4, 2024
@digiwand digiwand added regression-RC-12.4.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 5, 2024
@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
digiwand added a commit that referenced this issue Sep 17, 2024
…erFrom simulation "Spending cap" (#26684)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

Fixes PermitSingle, PermitBatch, PermitTransferFrom,
PermitBatchTransferFrom simulation to use their respective provided
token and amount in token details.

Additionally:
- adjusts styles to have an 8px gap between each "Spending Cap" entry
and right align content
- create PermitSimulationValueDisplay component to separate individual
token contract value logic
- adds tests
- adds sentry error to capture exception in case token isn't provided
for the relevant primaryTypes

dev note: majority of the newlines come from updated and new test
snapshots

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26684?quickstart=1)

Fixes: #26591
(PermitSingle)
Fixes: #26592
(PermitBatch)

Test PermitSingle & PermitBatch with Improved Signature setting enabled

See related issues for repro steps

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<img width="320"
src="https://github.com/user-attachments/assets/aac7fa8c-285a-4d2b-bcec-84c7fc6245bf">
<!-- [screenshots/recordings] -->
See Issue screenshots

<!-- [screenshots/recordings] -->

![image](https://github.com/user-attachments/assets/4564530d-1c3d-430b-b821-15ed0fca6152)

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.
digiwand added a commit that referenced this issue Sep 20, 2024
…erFrom simulation "Spending cap" (#26684)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

Fixes PermitSingle, PermitBatch, PermitTransferFrom,
PermitBatchTransferFrom simulation to use their respective provided
token and amount in token details.

Additionally:
- adjusts styles to have an 8px gap between each "Spending Cap" entry
and right align content
- create PermitSimulationValueDisplay component to separate individual
token contract value logic
- adds tests
- adds sentry error to capture exception in case token isn't provided
for the relevant primaryTypes

dev note: majority of the newlines come from updated and new test
snapshots

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26684?quickstart=1)

Fixes: #26591
(PermitSingle)
Fixes: #26592
(PermitBatch)

Test PermitSingle & PermitBatch with Improved Signature setting enabled

See related issues for repro steps

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<img width="320"
src="https://github.com/user-attachments/assets/aac7fa8c-285a-4d2b-bcec-84c7fc6245bf">
<!-- [screenshots/recordings] -->
See Issue screenshots

<!-- [screenshots/recordings] -->

![image](https://github.com/user-attachments/assets/4564530d-1c3d-430b-b821-15ed0fca6152)

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.
@digiwand digiwand reopened this Sep 20, 2024
@digiwand digiwand added regression-RC-12.3 Regression bug that was found in release candidate (RC) for release 12.3 and removed regression-RC-12.4.0 labels Sep 20, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Sep 20, 2024
@digiwand digiwand added regression-RC-12.3.0 Regression bug that was found in release candidate (RC) for release 12.3.0 and removed regression-RC-12.3 Regression bug that was found in release candidate (RC) for release 12.3 labels Sep 20, 2024
danjm pushed a commit that referenced this issue Sep 20, 2024
…rom, PermitBatchTransferFrom simulation "Spending cap" (#27300)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Cherry-picks #26684
into V12.3.0

resolved conflicts:
- permit-simulation (does not have static simulation yet)
- tests + snaps (12.3.0 does not have useConfirmContext yet)
<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27300?quickstart=1)

## **Related issues**

Fixes: #26591
(PermitSingle)
Fixes: #26592
(PermitBatch)

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-RC-12.3.0 Regression bug that was found in release candidate (RC) for release 12.3.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 release-blocker This bug is blocking the next release team-confirmations Push issues to confirmations team type-bug
Projects
Archived in project
4 participants