Owner-level ERC721 Permit #153

saucepoint · 2024-07-15T21:53:47Z

Owner-level ERC721 Permit (instead of token-level), which reduces frequent cold-storage writes

replaces #139

* consolidate using owner, update burn * fix: accrue deltas to caller in increase

* rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file

snreynolds

Nice! Love the tests too. Just some cleanup ideas - happy for them to be done in a later PR if you can keep track, up to you

snreynolds · 2024-07-17T16:39:26Z

contracts/NonfungiblePositionManager.sol


        // 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)});


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

snreynolds · 2024-07-17T16:39:48Z

contracts/NonfungiblePositionManager.sol

+    }
+
+    function getApproved(uint256 tokenId) public view override returns (address) {
+        require(_exists(tokenId), "ERC721: approved query for nonexistent token");


custom error/revert

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 _isApprovedOrOwner.. its isApprovedForAll and I believe that is the actual ERC72 spec

And I think we can move all this logic into ERC721Permit contract

snreynolds · 2024-07-17T16:46:58Z

contracts/base/ERC721Permit.sol

        external
        payable
        override
    {
        require(block.timestamp <= deadline, "Permit expired");

-        bytes32 digest = keccak256(


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)

snreynolds · 2024-07-17T17:08:05Z

contracts/base/ERC721Permit.sol

@@ -46,23 +45,18 @@ abstract contract ERC721Permit is ERC721, IERC721Permit {
        0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad;


I prefer saving the constant as keccak256(string..)

also can we move all this to a hashing library? PermitHash.hash(spender, tokenId, nonce, deadline)?

And then we can build another library to verify the signature after the hash

so permit code looks something like:

function permit() checkDeadline(deadline) { _useNonce() bytes32 permitHash = PermitHash.hash(spender, tokenId, deadline, nonce); signature.verify(hash, owner); // handles sc signatures as well _approve(operator) }

snreynolds · 2024-07-17T17:09:01Z

contracts/base/ERC721Permit.sol

+    /// @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();
+    }


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?

* use position salts * use fees owed in some tests * remove claims from increase,decrease * increment token id before reading it * Revert "increment token id before reading it" This reverts commit d366d75. * owner to alice * add more mint gas tests * update comment * Owner-level ERC721 Permit (#153) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce * tests for operator/permit * snapshots * gas benchmarks for permit * test fixes * unordered nonces * fix tests / cheatcode usage * fix tests --------- Co-authored-by: Sara Reynolds <[email protected]> --------- Co-authored-by: gretzke <[email protected]> Co-authored-by: saucepoint <[email protected]>

* initial thoughts lock and batch * update safecallback with constructor * simple batch under lock * oops * misc version bump; will conflict but can resolve later * defining types and different levels of abstractions * merge in main; resolve conflicts * wip * misc fixes with main:latest * basic mint * begin moving tests to fuzz * test for slippage * burning * decrease liquidity * mint transfer burn, liquidityOf accounting * wip * refactor to use CurrencySettleTake * basic fee collection * wip * misc fix * fee collection for independent same-range parties * LiquidityPosition -> LiquidityRange * erc20 fee collection * decrease liquidity with fee collection * wip test decrease liquidity on same range * reworked fuzzers; more testing on fee claims for liquidity decreasing * forge fmt * test fixes for flipped deltas * wip * test coverage for increase liquidity cases * preliminary gas benchmarks * Position manager refactor (#2) * chore: update v4-core:latest (#105) * update core * rename lockAcquired to unlockCallback * update core; temporary path hack in remappings * update v4-core; remove remapping * wip: fix compatibility * update core; fix renaming of swap fee to lp fee * update core; fix events * update core; address liquidity salt and modify liquidity return values * fix incorrect delta accounting when modifying liquidity * fix todo, use CurrencySettleTake * remove deadcode * update core; use StateLibrary; update sqrtRatio to sqrtPrice * fix beforeSwap return signatures * forge fmt; remove commented out code * update core (wow gas savings) * update core * update core * update core; hook flags LSB * update core * update core * chore: update v4 core (#115) * Update v4-core * CurrencySettleTake -> CurrencySettler * Snapshots * compiling but very broken * replace PoolStateLibrary * update currency settle take * compiling * wip * use v4-core's forge-std * test liquidity increase * additional fixes for collection and liquidity decrease * test migration * replace old implementation with new --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> * cleanup: TODOs and imports * Position manager Consolidate (#3) * wip: consolidation * further consolidation * consolidate to single file * yay no more stack too deep * some code comments * use currency settler syntax * use v4-core's gas snapshot * use snapLastCall and isolate for posm benchmarks * Update contracts/libraries/CurrencySettleTake.sol Co-authored-by: 0x57 <[email protected]> * use v4-core's solmate its more recent * use v4-core's openzeppelin-contracts * add ERC721Permit * feedback: memory hookData * initial refactor. stack too deep * passing tests * gutted LockAndBatchCall * cleanup diff * renaming vanilla functions * sanitize * change add liq accounting (#126) * change add liq accounting * remove rand comments * fix exact fees * use closeAllDeltas * comments cleanup * additional liquidity tests (#129) * additional increase liquidity tests * edge case of using cached fees for autocompound * wip * fix autocompound bug, use custodied and unclaimed fees in the autocompound * fix tests and use BalanceDeltas (#130) * fix some assertions * use BalanceDeltas for arithmetic * cleanest code in the game??? * additional cleaning * typo lol * autocompound gas benchmarks * autocompound excess credit gas benchmark * save 600 gas, cleaner code when moving caller delta to tokensOwed --------- Co-authored-by: saucepoint <[email protected]> * create compatibility with arbitrary execute handler * being supporting batched ops on vanilla functions * some initial tests to drive TDD * gas with isolate * mint to recipient * refactor for external call and code reuse (#134) * updated interface with unlockAndExecute * update decrease (#133) * update decrease * update collect * update decrease/collect * remove delta function * update burn * fix bubbling different return types because of recursive calls * all operations only return a BalanceDelta type (#136) * temp-dev-update (#135) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * fix: cleanup --------- Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> * using internal calls, first pass (#143) * using internal calls, first pass * fix gas tests * fix execute test * fix tests * edit mint gas test * fix mint test * fix warnings * dumb fix to test ci (#146) * dumb fix to test ci * memory limit * update gas limit to pass tests --------- Co-authored-by: gretzke <[email protected]> * some more gas snapshots (#150) * feat: posm, use salts for all positions & update permit (#148) * use position salts * use fees owed in some tests * remove claims from increase,decrease * increment token id before reading it * Revert "increment token id before reading it" This reverts commit d366d75. * owner to alice * add more mint gas tests * update comment * Owner-level ERC721 Permit (#153) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce * tests for operator/permit * snapshots * gas benchmarks for permit * test fixes * unordered nonces * fix tests / cheatcode usage * fix tests --------- Co-authored-by: Sara Reynolds <[email protected]> --------- Co-authored-by: gretzke <[email protected]> Co-authored-by: saucepoint <[email protected]> * Multicall & initialize (#154) * add multicall and an external function for initialization, with tests * test multicall contract * gas snapshot multicall * fix ci test * fix tests * forge fmt * change how msg.value is used in multicall mock --------- Co-authored-by: Sara Reynolds <[email protected]> * prep shared actions (#158) * update main (#162) * ERC721Permit cleanup (#161) * wip Solmate ERC721 * the queens dead, you may put down your arms. with this commit, i hereby lay BaseLiquidityManagement and the ideals of fee accounting finally to rest * refactor tokenId => LiquidityRange; begin separating PoolKey * move comment --------- Co-authored-by: Sara Reynolds <[email protected]> * remove old files, imports * Update src/NonfungiblePositionManager.sol Co-authored-by: saucepoint <[email protected]> * pr comments * pr comments * refactor test helpers per feedback * fix gas * remove permit * fix compiler warnings * rename to PositionManager * cache length * skip take for 0 * fix tests * Update src/interfaces/IPositionManager.sol Co-authored-by: Alice <[email protected]> * update multicall tests per feedback * remove unused imports * more unused imports * improve assertion * assert mint recipient is the payer and not the recipient * pr feedback * assert pool creation * use poolManager * remove liquidityrange imports * remove version string * pr comments, use base test setup * fuzz sqrtPrice * use fuzz, assert eq * other final rand pr comment fixes * fix off by 1 * use bound * use nextTokenId --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: gretzke <[email protected]> Co-authored-by: Alice <[email protected]>

saucepoint and others added 19 commits June 28, 2024 11:23

checkpointing

8ce4ed6

move decrease and collect to transient storage

5f7e9c1

remove returns since they are now saved to transient storage

5d780c8

draft: delta closing

c08bef1

merge in unified return types

2f42584

wip

a2186d7

Sra/edits (#137)

0f39420

* consolidate using owner, update burn * fix: accrue deltas to caller in increase

Rip Out Vanilla (#138)

ae001d0

* rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file

move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce

595bd87

tests for operator/permit

0f59d0f

snapshots

0722377

gas benchmarks for permit

190adfc

merge in dev-posm

9e9d2c0

test fixes

8f50bdc

unordered nonces

ab74982

merge in dev; still breaking

4adc425

fix tests / cheatcode usage

dc5d4d4

merge in core salts; broken tests

3fbe616

fix tests

1ba63de

saucepoint added the posm position manager label Jul 16, 2024

snreynolds reviewed Jul 17, 2024

View reviewed changes

merge in changes; resolve conflicts; regenerate gas

b6e1605

snreynolds merged commit af1f50a into posm-core-accounting Jul 17, 2024
3 checks passed

snreynolds deleted the posm-permit branch July 17, 2024 19:34

snreynolds mentioned this pull request Jul 17, 2024

PosM: owner-level nonce for permit #139

Closed

saucepoint mentioned this pull request Jul 18, 2024

ERC721Permit cleanup #161

Merged

snreynolds mentioned this pull request Jul 19, 2024

[posm]: Add Permit functionality to ERC721 #167

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Owner-level ERC721 Permit #153

Owner-level ERC721 Permit #153

saucepoint commented Jul 15, 2024

snreynolds left a comment

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

snreynolds Jul 17, 2024

		@@ -46,23 +45,18 @@ abstract contract ERC721Permit is ERC721, IERC721Permit {
		0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad;

Owner-level ERC721 Permit #153

Owner-level ERC721 Permit #153

Conversation

saucepoint commented Jul 15, 2024

snreynolds left a comment

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment

snreynolds Jul 17, 2024

Choose a reason for hiding this comment