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: Adding typed sign support for NFT permit #27796

Merged
merged 11 commits into from
Oct 16, 2024
Merged

feat: Adding typed sign support for NFT permit #27796

merged 11 commits into from
Oct 16, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Oct 11, 2024

Description

Adding support for NFT permit signature request.

Related issues

Fixes: #27396

Manual testing steps

  1. Submit an NFT permit request
  2. Check the confirmation page that appears

Screenshots/Recordings

Screenshot 2024-10-11 at 7 43 59 PM

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.

@jpuri jpuri added the team-confirmations Push issues to confirmations team label Oct 11, 2024
@jpuri jpuri requested review from a team as code owners October 11, 2024 14:27
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.

matthewwalsh0
matthewwalsh0 previously approved these changes Oct 11, 2024
confirmation: SignatureRequestType,
) => {
if (!confirmation) {
return {};
Copy link
Member

Choose a reason for hiding this comment

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

Minor, would it be more explicit to return undefined here so we can insist primary type and standard are always returned?

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 do not get this point exactly, I return empty object here for ease of destructuring the result.

ui/pages/confirmations/hooks/useTypedSignSignatureInfo.ts Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [d002def]
Page Load Metrics (1714 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14942330171016981
domContentLoaded14862303168516881
load14942329171417182
domInteractive15195503818
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.42 KiB (0.03%)
  • common: 98 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Oct 15, 2024
if (primaryType !== TypedSignSignaturePrimaryTypes.PERMIT) {
return undefined;
}
console.log(confirmation, confirmation?.msgParams?.data);
Copy link
Member

Choose a reason for hiding this comment

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

Accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
9.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@pedronfigueiredo
Copy link
Contributor

Would it make sense to add an e2e test?

@jpuri jpuri enabled auto-merge October 16, 2024 11:07
@jpuri
Copy link
Contributor Author

jpuri commented Oct 16, 2024

@pedronfigueiredo : yep that is good idea, let me do that.

@jpuri jpuri added this pull request to the merge queue Oct 16, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [f8f74d0]
Page Load Metrics (1861 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14702170186316680
domContentLoaded14602149183015775
load14732175186116579
domInteractive1694412010
backgroundConnect1282322010
firstReactRender55293975526
getState563232210
initialActions01000
loadScripts10521697136114971
setupStore1199372814
uiStartup164625382104216104
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.35 KiB (0.03%)
  • common: 98 Bytes (0.00%)

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@jpuri jpuri added this pull request to the merge queue Oct 16, 2024
Merged via the queue into develop with commit bf87d72 Oct 16, 2024
76 of 78 checks passed
@jpuri jpuri deleted the nft_permit branch October 16, 2024 15:16
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 16, 2024
@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ERC-721 Permit Signatures
5 participants