-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fix: isPermitSignatureRequest logic. Do not display as Permit if spender is not present #27384
Conversation
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. |
Builds ready [7cd71ee]
Page Load Metrics (1809 ± 126 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
e.g. owner and value will not exist if it is a Permit2 request.
Quality Gate passedIssues Measures |
Builds ready [876f27d]
Page Load Metrics (1732 ± 76 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Hey @digiwand : I think this task requires more detailed discussion as there can be many different validations for different signature. In case we are not able to validate a signature - should we reject it or display as simple typed sign or some other approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should re-evaluate the approach and discuss this more.
Hi @jpuri, I agree we should discuss how to approach this further. In the meantime, I think we should still enforce the
I think we should persist the behavior of our old signatures and not reject the signatures in this case. As the signature authors design their own EIP-712 struct, any arbitrary string could be a valid We could possibly warn developers if we detect that they may be trying to create a EIP-2612 signature and the required fields are invalid or missing. We have explicit support for a minor subset of Permit |
per discussion to reevaulate UI/UX closing ticket |
Description
If primaryType either:
and it does not have a "spender" hex address, do not render the signature as a Permit. Without a spender, we will fallback to an EIP-712 signature.
Note: The check for isPermitSignatureRequest is not sophisticated. This will be revisited with the upcoming Signatures API work.
Related issues
Fixes: #27385
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist