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

SignatureVerification.verify is not documented #201

Open
PaulRBerg opened this issue May 15, 2023 · 3 comments
Open

SignatureVerification.verify is not documented #201

PaulRBerg opened this issue May 15, 2023 · 3 comments

Comments

@PaulRBerg
Copy link
Contributor

PaulRBerg commented May 15, 2023

Problem

Given that cryptography is a one-way street, the place where Permit2 users will most frequently encounter errors is the verify function of the SignatureVerification library:

function verify(bytes calldata signature, bytes32 hash, address claimedSigner) internal view {
bytes32 r;
bytes32 s;
uint8 v;

The function, unfortunately, lacks any form of guidance, leaving users on their own to source the necessary documentation.

This a problem that is compounded by Uniswap's leading position in DeFi, as the signature verification processes in Permit2 may be the first encounter with cryptography for many developers, especially front-end developers who have just made a career switch to web3.

Suggestions

  • Provide a high-level English description of the function's algorithm by using NatSpec comments (@notice, @dev, @param, and so forth)
  • Make the implicit knowledge in this function explicit, i.e., explicitly mention the fact that the signature param is an ECDSA signature, and that the typical secp256k1 curve is what powers your cryptography
  • Explain what the benefit of using both standard ECDSA signatures and EIP-2098 signatures is; also, explain that users can use either signing scheme, and that supporting both or just one scheme is up to them
  • Explain that using EIP-1271 means signatures can be signers, too
  • Document and explain UPPER_BIT_MASK (consider adding a EIP1271_ prefix)
  • For convenience, provide explicit link to EIP-2098 and EIP-1271
  • Consider reducing the control flow nesting depth by flipping the claimedSigner.code.length == 0 check into claimedSigner.code.length > 0, and using return to stop the function execution early. Here's a code snippet that shows what I mean.

Additional Context

We (cc @gavriliumircea, @razgraf) are currently working on integrating Permit2 into our project (Sablier V2), and have recently spent a lot of time debugging a bug that was being triggered by a revert on line 41 in SignatureVerification:

if (signer != claimedSigner) revert InvalidSigner();

The bug was caused by an issue in our front-end; however, it would have helped us during debugging to have a little more guidance in SignatureVerification.verify.

@gavriliumircea
Copy link

I do agree with @PaulRBerg and further more I think it would be helpful, for the frontend developers, to create a guideline for frontend integrations as well.( Right now there a literally almost no resources at all regarding this matter.)

@PaulRBerg
Copy link
Contributor Author

create a guideline for frontend integrations

Yep. A front-end template that shows how to use Permit2 with Ethers.js would go a long way.

@razgraf
Copy link

razgraf commented May 16, 2023

create a guideline for frontend integrations

Yep. A front-end template that shows how to use Permit2 with Ethers.js would go a long way.

The permit2-sdk library already acts as some sort of a template (esp. when paired with the interfaces repository). What it does lack is explicit documentation of a full end-to-end flow, which is the real bummer here. Suggestions on how to "extend" the functionality at a contract level (calling .permit(), passing structured params) would go a long way as well. It would help avoid certain misconceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants