feat(Permit3): unlock account message type #2
Conversation
| usedNonces[owner][nonce] = 1; | ||
| function _useNonce(address owner, bytes32 salt) internal { | ||
| require(usedNonces[owner][salt] == 0, NonceAlreadyUsed()); | ||
| usedNonces[owner][salt] = 1; |
There was a problem hiding this comment.
using both salt and nonce in these methods is confusing
There was a problem hiding this comment.
salt is what user passes, but salt is userAddress + salt
There was a problem hiding this comment.
but I agree it's not elegant
There was a problem hiding this comment.
lol even in that response it is confusing
There was a problem hiding this comment.
We should unify into a single name — I see the conflict because it is kind of a salt and definitely a nonce. We can probably go with salt or something like snonce? Also could make the name descriptive like asyncNonce
pepper is taken...
| @@ -83,6 +85,7 @@ contract Permit3 is IPermit3, PermitBase, NonceManager { | |||
| */ | |||
| function permit( | |||
| address owner, | |||
| bytes32 salt, | |||
src/Permit3.sol
Outdated
| @@ -114,32 +117,40 @@ contract Permit3 is IPermit3, PermitBase, NonceManager { | |||
| * 1. Immediate transfers (transferOrExpiration = 1) | |||
| * 2. Allowance updates (transferOrExpiration = future timestamp) | |||
| if (allowed.expiration == LOCKED_ALLOWANCE && timestamp < allowed.timestamp) { | ||
| if ( | ||
| allowed.expiration == LOCKED_ALLOWANCE | ||
| && (p.modeOrExpiration != uint48(PermitType.Unlock) || timestamp <= allowed.timestamp) |
There was a problem hiding this comment.
What if i wanted to increase the timestamp of the lock?
src/NonceManager.sol
Outdated
| usedNonces[msg.sender][noncesToInvalidate[i]] = 1; | ||
| uint256 length = salts.length; | ||
|
|
||
| for (uint256 i = 0; i < length; i++) { |
There was a problem hiding this comment.
i don't think this matters here
There was a problem hiding this comment.
5 gas saved 🙏
| allowed.timestamp = timestamp; | ||
| } | ||
| } | ||
|
|
||
| emit Permit(owner, p.token, p.spender, allowed.amount, allowed.expiration, chain.nonce); | ||
| emit Permit(owner, p.token, p.account, allowed.amount, allowed.expiration, timestamp); |
src/interfaces/IPermit.sol
Outdated
| @@ -80,15 +80,15 @@ interface IPermit { | |||
| * @param spender The approved spender | |||
| * @param amount The approved amount | |||
| * @param expiration When the approval expires | |||
| * @param nonce The nonce used in the permit signature | |||
| * @param timestamp The nonce used in the permit signature | |||
| bytes32 chainedInvalidationHashes = proof.preHash; | ||
| chainedInvalidationHashes = | ||
| keccak256(abi.encodePacked(chainedInvalidationHashes, hashNoncesToInvalidate(proof.invalidations))); | ||
| bytes32 unbalancedInvalidationRoot = proof.preHash; |
There was a problem hiding this comment.
having trouble wrapping my head around this, but i think i just need to think about it more
|
|
||
| 4. **Increase Mode** (`transferOrExpiration > 2`) | ||
| 4. **Unlock Mode** (`transferOrExpiration = 3`) |
There was a problem hiding this comment.
glad we added this explicitly
| @@ -101,9 +105,9 @@ struct Allowance { | |||
| ### Account Locking | |||
|
|
|||
| Locked accounts have special restrictions: | |||
| - Cannot increase allowances | |||
| - Cannot increase/decrease allowances | |||
There was a problem hiding this comment.
we want to keep the ability to decrease allowances in this state — if a user has approvals out for N tokens they might want to be able to decrease them all to zero while in this state
I guess you could pass in an array with an unlock step first and set all the approvals to zero in the operations that follow, but I like the idea that you can ensure the allowances are decreased in lock state + its more friendly to non-dev users
There was a problem hiding this comment.
disregard: I now see that lockdown is per token and therefore this is not needed
There was a problem hiding this comment.
maybe add a comment here that lockdown is per token and sets allowances to zero — that was why I was confused at first
| bytes32 chainedInvalidationHashes = proof.preHash; | ||
| chainedInvalidationHashes = | ||
| keccak256(abi.encodePacked(chainedInvalidationHashes, hashNoncesToInvalidate(proof.invalidations))); | ||
| bytes32 unbalancedInvalidationRoot = proof.preHash; |
There was a problem hiding this comment.
unbalancedInvalidationRoot --> unhingedInvalidationRoot 🥇
| usedNonces[owner][nonce] = 1; | ||
| function _useNonce(address owner, bytes32 salt) internal { | ||
| require(usedNonces[owner][salt] == 0, NonceAlreadyUsed()); | ||
| usedNonces[owner][salt] = 1; |
There was a problem hiding this comment.
We should unify into a single name — I see the conflict because it is kind of a salt and definitely a nonce. We can probably go with salt or something like snonce? Also could make the name descriptive like asyncNonce
pepper is taken...
src/Permit3.sol
Outdated
| @@ -27,15 +27,16 @@ contract Permit3 is IPermit3, PermitBase, NonceManager { | |||
| * Used in cross-chain signature verification | |||
| */ | |||
| bytes32 public constant CHAIN_PERMITS_TYPEHASH = keccak256( | |||
| "ChainPermits(uint64 chainId,uint48 nonce,AllowanceOrTransfer[] permits)AllowanceOrTransfer(uint48 transferOrExpiration,address token,address spender,uint160 amountDelta)" | |||
| "ChainPermits(uint64 chainId,AllowanceOrTransfer[] permits)AllowanceOrTransfer(uint48 transferOrExpiration,address token,address spender,uint160 amountDelta)" | |||
There was a problem hiding this comment.
transferOrExpiration --> modeOrExpiration
There was a problem hiding this comment.
also spender should be account
|
I noticed some of the variable names are different in the EIP.md still — so will want to rectify those |
No description provided.