ERC721Permit #210
ERC721Permit #210
Conversation
|
|
||
| import {PosmTestSetup} from "../shared/PosmTestSetup.sol"; | ||
|
|
||
| contract PermitTest is Test, PosmTestSetup { |
There was a problem hiding this comment.
can we call this PermitERC721 ? Cause we will also have a permit for permit2 exposed
There was a problem hiding this comment.
will rename this at the end before merge to preserve reviews / diffs
src/base/UnorderedNonce.sol
Outdated
| mstore(0, 0x1fb09b80) // 4 bytes of NonceAlreadyUsed.selector | ||
| revert(0x1c, 0x04) |
There was a problem hiding this comment.
if we're going to use .revertWith lib from core this will need to be changed
There was a problem hiding this comment.
we can put that in the error pr just a call out tho
src/base/UnorderedNonce.sol
Outdated
| function bitmapPositions(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) { | ||
| wordPos = uint248(nonce >> 8); | ||
| bitPos = uint8(nonce); | ||
| function _flipBit(mapping(uint256 => uint256) storage bitmap, uint256 nonce) private { |
There was a problem hiding this comment.
imo this does more than flip the bit so we should maybe split out this code.
1 - use the nonce to extract the wordPos/the bitmap to operate on
2- flip bit
flipBit(nonce) makes it seem like the nonce thats being passed in is the bit thats being flipped which is not really how the data is used
src/base/UnorderedNonce.sol
Outdated
| sstore(slot, or(previousBits, bit)) | ||
| } | ||
| (uint256 wordPos, uint256 bitPos) = nonce.getBitmapPositions(); | ||
| nonces[owner].flipBit(wordPos, bitPos); |
There was a problem hiding this comment.
is it more expensive if we did nonces[owner][wordPos].flipBit(bitPos)? can you see? i think i just like that abstraction a little better
There was a problem hiding this comment.
its not as friendly since flipBit would calculate the new bitmap and return it so the caller can set it in storage
the alternative/cleaner syntax might be nonces[owner].spendNonce(nonce) where spendNonce does the wordPos/bitPos logic
There was a problem hiding this comment.
ok i misunderstood - now i see it lol
i agree that doing it on the word would be messy becaus eyou have to write
nonces[owner][wordPos] = nonces[owner][wordPos].flipBit(x)
There was a problem hiding this comment.
i think i still prefer the abstraction where we were to change it to
nonces[owner][wordPos].flipTick bc the functions become more reusable (to extract bitPos and wordPos) from nonce, then to get the new bitmap...
buttt i get it if its too expensive
maybe we could still have nonces[owner].spendNonce() that still use other helpers for those two things? im just noticing that we have to rewrite helpers in tests to do the same thing weve written here/ I could see those functions being helpful generally in a library
There was a problem hiding this comment.
Can you see how much gas is saved not doing it this way? @saucepoint
like is it significantly more expsenive to do
nonces[owner][wordPos] = nonces[owner][wordPos].flipTick(bitPos) ?
There was a problem hiding this comment.
i will also say tho.. this is not a super heavily used codepath so we could just go back to the solidity version and be done w it, theres no need to hyper optimize this path
There was a problem hiding this comment.
yeah im happy with that 🤷 no strong opinions
| contract UnorderedNonce { | ||
| error NonceAlreadyUsed(); | ||
|
|
||
| mapping(address owner => mapping(uint256 word => uint256 bitmap)) public nonces; |
There was a problem hiding this comment.
natspec that the word is actually max type(uint248).max
test/position-managers/Permit.t.sol
Outdated
| ); | ||
| } | ||
|
|
||
| function test_permit_increaseLiquidity() public { |
There was a problem hiding this comment.
you dont actually need permissions to increase, so can probably remove this?
There was a problem hiding this comment.
or you can leave it to show you dont need a permit
There was a problem hiding this comment.
removing
the test test_increaseLiquidity_withUnapprovedCaller shows that users can increase without approval
| vm.prank(alice); | ||
| mint(config, liquidityAlice, alice, ZERO_BYTES); | ||
| uint256 tokenIdAlice = lpm.nextTokenId() - 1; | ||
|
|
There was a problem hiding this comment.
can you add an expect revert bob trying to adjust alices position before the permit
There was a problem hiding this comment.
tested in test_noPermit_decreaseLiquidityRevert
snreynolds
dismissed
hensha256’s stale review
alice has reviewed, will flag for another look Sunday
Related Issue
Closes #167 and replaces #205
Description of changes
Adds ERC721Permit with