feat: added contracts/evm for evm permit2 update#915
feat: added contracts/evm for evm permit2 update#915CarsonRoscoe wants to merge 22 commits intomainfrom
Conversation
🟡 Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CarsonRoscoe
force-pushed
the
feat/exact-evm-permit-2
branch
from
bb465c0 to
54a250b
Compare
CarsonRoscoe
changed the title
CarsonRoscoe
changed the title
CarsonRoscoe
force-pushed
the
feat/exact-evm-permit-2
branch
from
54a250b to
fe9a520
Compare
CarsonRoscoe
requested review from
fabrice-cheng
and removed request for
fabrice-cheng
| // Validate time window | ||
| if (block.timestamp < witness.validAfter) revert PaymentTooEarly(); | ||
| if (block.timestamp > witness.validBefore) revert PaymentExpired(); |
There was a problem hiding this comment.
Natspec above says witness.validAfter is the earliest timestamp that is acceptable. Thus, control should be expressed using <=. Same for validBefore.
There was a problem hiding this comment.
I think there was slight confusion in reviewing this, please verify my understanding.
if (block.timestamp < witness.validAfter) revert PaymentTooEarly() means that if the current timestamp is earlier than the earliest valid timestamp, revert
Similarly, if (block.timestamp > witness.validBefore) revert PaymentExpired() means that if the current timestamp is later than the last valid timestamp, revert
I believe that is correct, as <= and >= would then revert when equal, becoming exclusive. We want it to be inclusive
There was a problem hiding this comment.
Oh man, yep you got it right. We're describing the revert edges. My bad!
| // Validate amount | ||
| if (amount > permit.permitted.amount) revert AmountExceedsPermitted(); |
There was a problem hiding this comment.
Should be amount >= permit.permitted.amount
There was a problem hiding this comment.
I believe the current code if (amount > permit.permitted.amount) is correct. This allows amount == permitted.amount (exact permitted amount) and only reverts when exceeding. Using >= would reject exact matches, breaking valid use cases where users want to transfer exactly what they permitted.
|
Missing dep on OZ's reentrancy guard. Need to |
| // Emit event for observability | ||
| emit X402PermitTransfer(owner, transferDetails.to, transferDetails.requestedAmount, permit.permitted.token); |
There was a problem hiding this comment.
I think all the data emitted here is redundant considering the events that will be emitted by the ERC20 contract. Is there a specific reason we need to emit this data ourselves?
There was a problem hiding this comment.
Good points. Reached out to the data scientist who will eventually be observing these contracts to get his opinion on if he wants/needs any events or not, given the redundancy. Will let his decision guide what we emit
CarsonRoscoe
force-pushed
the
feat/exact-evm-permit-2
branch
from
49a756b to
ead5a1f
Compare
CarsonRoscoe
force-pushed
the
feat/exact-evm-permit-2
branch
from
0fab0ca to
b0a2b10
Compare
| event Settled(); | ||
|
|
||
| /// @notice Emitted when settleWithPermit() completes successfully | ||
| event SettledWithPermit(); |
There was a problem hiding this comment.
Do we really see a value of being able to distinguish Settled vs SettledWithPermit()?
There was a problem hiding this comment.
I don't believe it's strictly necessary, however I do prefer having it to future proof our observability.
Without these events, there is no event driven approach for a data scientist to determine which transactions are flowing through the 2612 path, or through a separate erc20 approval. I foresee us wanting to know the breakdowns of transfer methods in the future, which is why I added this
There was a problem hiding this comment.
100% agreed on this take ^
| * @dev Reverts if _permit2 is the zero address | ||
| */ | ||
| constructor( | ||
| address _permit2 |
There was a problem hiding this comment.
could you double check that adding this to the constructor with potential diff values won't affect the deployed contract address?
Unsure if the bytecode for CREATE2/CREATE3 is affected by constructor payload
There was a problem hiding this comment.
Good catch, a change in constructor arguments would indeed change the deployed address
There was a problem hiding this comment.
The constructor parameter I am planning to use is Uniswap's canonical Permit2 contract address. Based on these docs, the address should exist on 16 different mainnets, which is a decent range but also limiting.
We could resolve with a separate initialize(_permit2) function gated by an initializer modifier to ensure it's only called once, and add a initialized() modifier on the settle calls to ensure initialize() was called before the contract gets used
Thoughts on that @fabrice-cheng ?
|
|
||
| // Validate time window | ||
| if (block.timestamp < witness.validAfter) revert PaymentTooEarly(); | ||
| if (block.timestamp > witness.validBefore) revert PaymentExpired(); |
There was a problem hiding this comment.
fyi, Permit2 has a deadline in it, so this field would be redundant
`
struct PermitTransferFrom {
TokenPermissions permitted;
// a unique value for every token owner's signature to prevent signature replays
uint256 nonce;
// deadline on the permit signature
uint256 deadline;
}
`
There was a problem hiding this comment.
Great catch, addressing now
5 participants
THE CONTRACTS IN THIS PR HAVE YET TO BE AUDITED. ONLY DEPLOY TO TEST NETWORKS AT THIS TIME
Description
In order to implement #769, an
x402Permit2Proxycontract must be written and deployed.This PR adds a foundry project under
contracts/evm, which both acts as a foundry project for smart contract developmentBase Sepolia Deployment
Tests
Added united tests & base-sepolia forked integration tests
Checklist