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 permit2 support to SuperWETH and SuperchainERC20 #58

Closed
wants to merge 2 commits into from

Conversation

0xng
Copy link
Member

@0xng 0xng commented Sep 18, 2024

Description

Adds Permit2 support to SuperchainWETH and SuperchainERC20 contracts.

Information and Questions

Solady doesn't make use of its allowance function in transferFrom, instead it computes and reads from the slot directly. This means that overriding the allowance function doesn't affect the transferFrom function. For this reason I had to override transferFrom and add a special case for when PERMIT2 is the msg.sender. The logic within that if block is the same transferFrom performs when updating the balances of from and to.

Despite allowance not being used in transferFrom I still overrode it in case integrators fetch it to perform further logic. If I hadn't it would return 0 and that may mess up some of their logic.

When it came to SuperchainWETH I noticed it inherited from WETH98 which has allowance as a public mapping. This means it can't be overriden. I came up with two potential approaches:

  1. Following OZ's pattern of renaming allowance to _allowances and making it a private state variable. This allowed me to add an allowance getter and to make it virtual so I could override it easily. This also meant modifying a bit of logic from this contract and from DelayedWETH.
  2. Making transferFrom virtual and adding the case when PERMIT2 is the caller at the beginning.

I went for approach 2. However this approach doesn't reflect the virtual allowance of PERMIT2, so I'm not 100% sold this is the best approach. The main reason why I chose it was because it didn't affect the logic (other than adding a virtual keyword) of WETH98 nor any contract that inherited from it, and because I was not certain whether there were certain design guidelines that WETH98 had to follow.

I can implement the first approach quickly if needed. Also any other potential approached that I may have missed are welcomed.

One more doubt I had was whether to add this logic to the SuperchainERC20 or not. By adding it here we are forcing all contracts extending from it to support Permit2. Extension can then opt-out. Another approach would be to not add it to the SuperchainERC20 and have the extensions opt-in instead.

Lastly, these implementations and the current one on OptimismMintableERC20 doesn't have a way to revoke the infinite allowance. Are we sure we don't want to add this?

Copy link

github-actions bot commented Oct 4, 2024

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Oct 4, 2024
@github-actions github-actions bot closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant