-
Notifications
You must be signed in to change notification settings - Fork 508
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
Owner-level ERC721 Permit #153
Changes from 19 commits
8ce4ed6
5f7e9c1
5d780c8
c08bef1
2f42584
a2186d7
0f39420
ae001d0
595bd87
0f59d0f
0722377
190adfc
9e9d2c0
8f50bdc
ab74982
4adc425
dc5d4d4
3fbe616
1ba63de
b6e1605
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
177823 | ||
179921 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
192626 | ||
194724 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
192327 | ||
194425 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
135099 | ||
137197 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
135099 | ||
137197 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
165128 | ||
167226 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
165128 | ||
167226 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
444628 | ||
441402 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
490196 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
75049 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
57937 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
40849 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,7 +105,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit | |
|
||
(delta,) = _modifyLiquidity(range, liquidity.toInt256(), bytes32(tokenId), hookData); | ||
|
||
tokenPositions[tokenId] = TokenPosition({owner: owner, range: range}); | ||
tokenPositions[tokenId] = TokenPosition({owner: owner, range: range, operator: address(0x0)}); | ||
} | ||
|
||
// Note: Calling increase with 0 will accrue any underlying fees. | ||
|
@@ -159,19 +159,24 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit | |
TokenPosition storage tokenPosition = tokenPositions[tokenId]; | ||
LiquidityRangeId rangeId = tokenPosition.range.toId(); | ||
Position storage position = positions[from][rangeId]; | ||
position.operator = address(0x0); | ||
|
||
// transfer position data to destination | ||
positions[to][rangeId] = position; | ||
delete positions[from][rangeId]; | ||
|
||
// update token position | ||
tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range}); | ||
tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range, operator: address(0x0)}); | ||
} | ||
|
||
function _getAndIncrementNonce(uint256 tokenId) internal override returns (uint256) { | ||
TokenPosition memory tokenPosition = tokenPositions[tokenId]; | ||
return uint256(positions[tokenPosition.owner][tokenPosition.range.toId()].nonce++); | ||
// override ERC721 approval by setting operator | ||
function _approve(address spender, uint256 tokenId) internal override { | ||
tokenPositions[tokenId].operator = spender; | ||
} | ||
|
||
function getApproved(uint256 tokenId) public view override returns (address) { | ||
require(_exists(tokenId), "ERC721: approved query for nonexistent token"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. custom error/revert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, can we move to solmate ERC721? the interfaces are different. I think all the interfaces here are from an old OZ ERC721 version ie solmate ERC721 doesn't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I think we can move all this logic into ERC721Permit contract |
||
|
||
return tokenPositions[tokenId].operator; | ||
} | ||
|
||
modifier isAuthorizedForToken(uint256 tokenId, address sender) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,7 @@ import {IERC1271} from "../interfaces/external/IERC1271.sol"; | |
/// @title ERC721 with permit | ||
/// @notice Nonfungible tokens that support an approve via signature, i.e. permit | ||
abstract contract ERC721Permit is ERC721, IERC721Permit { | ||
/// @dev Gets the current nonce for a token ID and then increments it, returning the original value | ||
function _getAndIncrementNonce(uint256 tokenId) internal virtual returns (uint256); | ||
mapping(address owner => mapping(uint256 word => uint256 bitmap)) public nonces; | ||
|
||
/// @dev The hash of the name used in the permit signature verification | ||
bytes32 private immutable nameHash; | ||
|
@@ -46,23 +45,18 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { | |
0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer saving the constant as keccak256(string..) also can we move all this to a hashing library? And then we can build another library to verify the signature after the hash so permit code looks something like:
|
||
|
||
/// @inheritdoc IERC721Permit | ||
function permit(address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s) | ||
function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, uint8 v, bytes32 r, bytes32 s) | ||
external | ||
payable | ||
override | ||
{ | ||
require(block.timestamp <= deadline, "Permit expired"); | ||
|
||
bytes32 digest = keccak256( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO, lets tackle this in a separate PR for sure cause I want to do smaller PRs but there are some gas optimizations we can do by caching the DOMAIN_SEPERATOR and typehashes.. see this contract. I also think it will be nice to have that in its own contained contract. (And for reference I believe the original idea for this came from OZ https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol#L40) |
||
abi.encodePacked( | ||
"\x19\x01", | ||
DOMAIN_SEPARATOR(), | ||
keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _getAndIncrementNonce(tokenId), deadline)) | ||
) | ||
); | ||
address owner = ownerOf(tokenId); | ||
require(spender != owner, "ERC721Permit: approval to current owner"); | ||
|
||
bytes32 digest = getDigest(spender, tokenId, nonce, deadline); | ||
|
||
if (Address.isContract(owner)) { | ||
require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Unauthorized"); | ||
} else { | ||
|
@@ -71,6 +65,40 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { | |
require(recoveredAddress == owner, "Unauthorized"); | ||
} | ||
|
||
_useUnorderedNonce(owner, nonce); | ||
approve(spender, tokenId); | ||
} | ||
|
||
function getDigest(address spender, uint256 tokenId, uint256 _nonce, uint256 deadline) | ||
public | ||
view | ||
returns (bytes32 digest) | ||
{ | ||
digest = keccak256( | ||
abi.encodePacked( | ||
"\x19\x01", | ||
DOMAIN_SEPARATOR(), | ||
keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _nonce, deadline)) | ||
) | ||
); | ||
} | ||
|
||
/// @notice Returns the index of the bitmap and the bit position within the bitmap. Used for unordered nonces | ||
/// @param nonce The nonce to get the associated word and bit positions | ||
/// @return wordPos The word position or index into the nonceBitmap | ||
/// @return bitPos The bit position | ||
/// @dev The first 248 bits of the nonce value is the index of the desired bitmap | ||
/// @dev The last 8 bits of the nonce value is the position of the bit in the bitmap | ||
function bitmapPositions(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) { | ||
wordPos = uint248(nonce >> 8); | ||
bitPos = uint8(nonce); | ||
} | ||
|
||
function _useUnorderedNonce(address from, uint256 nonce) internal { | ||
(uint256 wordPos, uint256 bitPos) = bitmapPositions(nonce); | ||
uint256 bit = 1 << bitPos; | ||
uint256 flipped = nonces[from][wordPos] ^= bit; | ||
|
||
if (flipped & bit == 0) revert NonceAlreadyUsed(); | ||
} | ||
Comment on lines
+86
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice!! I imagine this was just taken directly from permit2? I wonder if we could actually build a UnorderedNonce library thats inheritable/useable for the future? |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no gas benefits to adding this to TokenPosition struct, I'm leaning towards taking it out, and having all ERC721 logic be separated from "Positions" information. Then we could have some kind of standardized ERC721Permit with all logic for operators and non incrementing nonce schemes. I also wonder if there is an ERC for allowing multiple operators on a tokenId... not saying we need it but let's add it to the TODOs as a potential improvement