Uniswap/permit2

`SignatureVerification.verify` is not documented

Opened this issue · 3 comments

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.

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.)

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.

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.