Prototype: Internal pool mapping and optimized pool lookup

[brianmcmichael]

Description of change

High level

• Enables usage of mint(pool) and isAjnaPool(pool) usage without requirement of subset hash.
• Add internal mapping of known valid subset (and non-subset) pools
• Allow for permissionless adding of valid pools to the subset table via a new addAjnaPool function. (Optional for non-subset pools)
• Optimize lookups of known Ajna pools within isAjnaPool function .
• Automatically add pools to the lookup table upon mint(pool, subsetHash) calls, reducing subsequent mint costs against that pool.
• Add lookup of non-subset ERC20 and ERC721 pools (and added ERC721 subset pools) without requiring subset hash.
• Add minting of non-subset ERC20 and ERC721 pools (and added ERC721 subset pools) without requiring subset hash.
• Gets non-subset hash from the factory as immutable upon PositionManager creation.
• Approximate savings of 20k gas per function call once a pool is added.

Description of bug or vulnerability and solution

Description of bug: Usability issue around subset hashes

• For subset ERC721 pools, the subset hash is going to be a general barrier for usability as the subsetHash is not available on the pool itself, nor is it emitted in an event on pool creation. The subsetHash for these pools will therefore need to be recreated by re-hashing the pool creation parameter calldata, or by introspecting the transaction itself. This missing data may in retrospect be a flaw in the design of the pools, however, for non-subset pools the hash is available on the factory contract itself and we can access that via a known method.
• This change keeps all of the original functionality of the existing PositionManager, but increases usability by not requiring that the user pass a subsetHash parameter for isAjnaPool and mint functions.
• Because the casual user does not have easy access to this variable, this modification would allow users to detect whether a pool is valid by simply passing the pool address to both isAjnaPool and mint, respectively.
• Once a pool is added to the known list of Ajna pools using the permissionless addAjnaPool(pool, subsetHash) function, all future lookups to isAjnaPool are optimized to short-circuit and return without additional external calls to the factory contract, this results in a gas savings of approximately 20183 gas per each call to isAjnaPool, these savings are reflected in direct calls to the function and in each mint call, which are passed along to the user.
• The contract can continue to be used as designed, and all existing tests are passing.
• A community keeper or interested party can call addAjnaPool for approximately 30k gas one time, saving 20k gas on each future call to the PositionManager for that pool. This allows one community member to calculate the subsetHash for an ERC721 subset pool in the position manager and add it for general use without the subsetHash requirement.

Cons:

• Function calls are increased by approximately 32 gas per call due to the additional mapping memory slot usage
• Prototype does not modify PositionManager interface contract
• ajnaPools mapping is internal, it may be useful for this to be public if this information is valuable elsewhere.
• Additional explicit tests of the new and existing functions are needed.

Demonstration of gas savings

It is difficult to see the impact of this change at a per-function level when comparing against the test gas report, so I am including the standard test reports with gas consumption reports to give a better estimate of gas savings on a typical transaction flow. I've also added a PositionManager2.t.sol file with the exact same test suite, but with the inclusion of the addAjnaPool in the test setup to demonstrate the value of the optimized lookups. Please compare the results against the current design.

Current PositionManager master

Running 26 tests for tests/forge/unit/Positions/PositionManager.t.sol:PositionManagerERC20PoolTest
[PASS] test3rdPartyMinter() (gas: 1574646)
[PASS] test3rdPartyMinterAndRedeemer() (gas: 1188602)
[PASS] testBurnNFTWithPositions() (gas: 1117005)
[PASS] testBurnNFTWithoutPositions() (gas: 103527)
[PASS] testDeployWith0xAddressRevert() (gas: 130114)
[PASS] testMayInteractReverts() (gas: 133500)
[PASS] testMemorializeAndRedeemBucketBankruptcy() (gas: 5239237)
[PASS] testMemorializeMultiple() (gas: 3247778)
[PASS] testMemorializePositions() (gas: 2128872)
[PASS] testMemorializePositionsTwoAccountsSameBucket() (gas: 1814625)
[PASS] testMint() (gas: 6031959)
[PASS] testMintToContract() (gas: 854766)
[PASS] testMoveLiquidity() (gas: 2998564)
[PASS] testMoveLiquidityPermissions() (gas: 1029077)
[PASS] testMoveLiquidityToOverwriteBankruptBucket() (gas: 6071524)
[PASS] testMoveLiquidityWithInterest() (gas: 3666643)
[PASS] testNFTTransferByApprove() (gas: 1266727)
[PASS] testNFTTransferByPermit() (gas: 1470349)
[PASS] testPermitByContract() (gas: 1112601)
[PASS] testPermitDuringTransfers() (gas: 1315636)
[PASS] testPermitReverts() (gas: 363980)
[PASS] testRedeemEmptyPositions() (gas: 124129)
[PASS] testRedeemPositions() (gas: 1176139)
[PASS] testRedeemPositionsByNewNFTOwner() (gas: 1918268)
[PASS] testRememorializePositions() (gas: 2564240)
[PASS] testTokenURI() (gas: 2169762)
Test result: ok. 26 passed; 0 failed; 0 skipped; finished in 10.72s

After change PositionManager.t.sol

This is the existing test suite result after the introduction of these changes. This does increase gas costs generally in the tested workflow. However, this workflow does not generally account for multiple mints or lookups against a pool. This assumes that the pool has not been added to the PositionManager and factory lookups are still required.

Running 29 tests for tests/forge/unit/Positions/PositionManager.t.sol:PositionManagerERC20PoolTest
[PASS] test3rdPartyMinter() (gas: 1593240)
[PASS] test3rdPartyMinterAndRedeemer() (gas: 1211713)
[PASS] testBurnNFTWithPositions() (gas: 1135600)
[PASS] testBurnNFTWithoutPositions() (gas: 121980)
[PASS] testDeployWith0xAddressRevert() (gas: 130435)
[PASS] testIsAjnaPool() (gas: 30506)
[PASS] testIsAjnaPoolAddedWithoutSubsetHash() (gas: 52529)
[PASS] testIsAjnaPoolWithoutSubsetHash() (gas: 31100)
[PASS] testMayInteractReverts() (gas: 156589)
[PASS] testMemorializeAndRedeemBucketBankruptcy() (gas: 5262568)
[PASS] testMemorializeMultiple() (gas: 3265759)
[PASS] testMemorializePositions() (gas: 2152247)
[PASS] testMemorializePositionsTwoAccountsSameBucket() (gas: 1828781)
[PASS] testMint() (gas: 6066065)
[PASS] testMintToContract() (gas: 877833)
[PASS] testMoveLiquidity() (gas: 3017777)
[PASS] testMoveLiquidityPermissions() (gas: 1047460)
[PASS] testMoveLiquidityToOverwriteBankruptBucket() (gas: 6089197)
[PASS] testMoveLiquidityWithInterest() (gas: 3680640)
[PASS] testNFTTransferByApprove() (gas: 1290146)
[PASS] testNFTTransferByPermit() (gas: 1493856)
[PASS] testPermitByContract() (gas: 1135822)
[PASS] testPermitDuringTransfers() (gas: 1339011)
[PASS] testPermitReverts() (gas: 392559)
[PASS] testRedeemEmptyPositions() (gas: 147196)
[PASS] testRedeemPositions() (gas: 1199536)
[PASS] testRedeemPositionsByNewNFTOwner() (gas: 1941731)
[PASS] testRememorializePositions() (gas: 2588583)
[PASS] testTokenURI() (gas: 2192939)
Test result: ok. 29 passed; 0 failed; 0 skipped; finished in 3.89s

After change PositionManager2.t.sol

This test suite is the same as PositionManager.t.sol above, with the addition of the pool to the PositionManager via addAjnaPool in the test setup. For all tests there is a reduction in cost of transaction flow across the board. Each pool only needs to be added to the PositionManager one time to achieve these savings. Once added, the flow cost of each test is less than in the current design. For popular pools, this could allow for a compounded gas savings for all users.

Running 29 tests for tests/forge/unit/Positions/PositionManager2.t.sol:PositionManagerERC20PoolTest
[PASS] test3rdPartyMinter() (gas: 1564801)
[PASS] test3rdPartyMinterAndRedeemer() (gas: 1177759)
[PASS] testBurnNFTWithPositions() (gas: 1107160)
[PASS] testBurnNFTWithoutPositions() (gas: 89541)
[PASS] testDeployWith0xAddressRevert() (gas: 130435)
[PASS] testIsAjnaPool() (gas: 10323)
[PASS] testIsAjnaPoolAddedWithoutSubsetHash() (gas: 11980)
[PASS] testIsAjnaPoolWithoutSubsetHash() (gas: 10905)
[PASS] testMayInteractReverts() (gas: 116040)
[PASS] testMemorializeAndRedeemBucketBankruptcy() (gas: 5227019)
[PASS] testMemorializeMultiple() (gas: 3230210)
[PASS] testMemorializePositions() (gas: 2116698)
[PASS] testMemorializePositionsTwoAccountsSameBucket() (gas: 1800342)
[PASS] testMint() (gas: 6039516)
[PASS] testMintToContract() (gas: 837284)
[PASS] testMoveLiquidity() (gas: 2982228)
[PASS] testMoveLiquidityPermissions() (gas: 1019021)
[PASS] testMoveLiquidityToOverwriteBankruptBucket() (gas: 6053648)
[PASS] testMoveLiquidityWithInterest() (gas: 3652201)
[PASS] testNFTTransferByApprove() (gas: 1254597)
[PASS] testNFTTransferByPermit() (gas: 1458307)
[PASS] testPermitByContract() (gas: 1095273)
[PASS] testPermitDuringTransfers() (gas: 1303462)
[PASS] testPermitReverts() (gas: 352010)
[PASS] testRedeemEmptyPositions() (gas: 106647)
[PASS] testRedeemPositions() (gas: 1163987)
[PASS] testRedeemPositionsByNewNFTOwner() (gas: 1906182)
[PASS] testRememorializePositions() (gas: 2553034)
[PASS] testTokenURI() (gas: 2157390)
Test result: ok. 29 passed; 0 failed; 0 skipped; finished in 152.47ms

Contract size

Pre Change

src/PositionManager.sol:PositionManager contract
Deployment Cost	Deployment Size
3852018	19835

Post Change

src/PositionManager.sol:PositionManager contract
Deployment Cost	Deployment Size
4035235	21024

Gas usage

Pre Change

src/PositionManager.sol:PositionManager contract
Deployment Cost	Deployment Size
3852018	19835
Function Name	min	avg	median	max	# calls
DOMAIN_SEPARATOR	732	732	732	732	9
PERMIT_TYPEHASH	426	426	426	426	8
approve(address,uint256)	25302	25302	25302	25302	31
approve(address,uint256)(bool)	2722	25225	25302	27302	60
burn	1838	5440	5431	11214	12
getLP	6558	10718	7854	36286	460
getPositionIndexes	1680	2983	2150	14620	454
getPositionIndexesFiltered	8687	28795	26399	87539	634
getPositionInfo	1053	2303	1053	5053	16
isAjnaPool	955	7292	6758	20758	58
isIndexInPosition	1066	1629	1066	3066	71
isPositionBucketBankrupt	6870	7886	8090	9658	18
memorializePositions	4459	84572	73461	1152856	3123
mint	10462	88099	95184	100184	127
moveLiquidity	5365	194327	165492	671265	19
nonces	841	1817	1746	2841	8
ownerOf	836	860	836	2836	333
permit	1290	23250	17585	44417	9
poolKey	807	1086	807	2807	86
redeemPositions	2855	38280	43008	141906	29
safeTransferFrom	39704	41224	41704	41704	5
tokenURI	3157	468592	468592	934028	2
transferFrom(address,address,uint256)	20212	26823	24136	41656	33
transferFrom(address,address,uint256)(bool)	20212	32061	34056	41656	93

Post Change

src/PositionManager.sol:PositionManager contract
Deployment Cost	Deployment Size
4035235	21024
Function Name	min	avg	median	max	# calls
DOMAIN_SEPARATOR	754	754	754	754	18
PERMIT_TYPEHASH	448	448	448	448	16
addSubsetPool	2916	42156	43465	43465	31
approve(address,uint256)	2722	24218	25302	27302	19
approve(address,uint256)(bool)	2722	25146	25302	27302	55
burn	1882	5481	5470	11249	24
getLP	6602	10911	8204	36330	467
getPositionIndexes	1724	4564	2664	14664	159
getPositionIndexesFiltered	8687	42839	40334	79312	90
getPositionInfo	1097	2097	1097	5097	20
isAjnaPool(address)(bool)	739	6787	1739	22934	4
isAjnaPool(address,bytes32)(bool)	807	1766	807	22990	52
isIndexInPosition	1110	1673	1110	3110	142
isPositionBucketBankrupt	6914	7963	8134	9702	29
memorializePositions	4481	83797	73483	1152873	3127
mint	21490	99159	118086	123086	136
moveLiquidity	5365	191163	163894	671265	29
nonces	841	1817	1746	2841	16
ownerOf	858	884	858	2858	315
permit	1334	23294	17629	44461	18
poolKey	851	1220	851	2851	65
redeemPositions	2899	38847	43044	141941	54
safeTransferFrom	39721	41187	41721	41721	9
tokenURI	3201	468636	468636	934072	4
transferFrom(address,address,uint256)	20229	30588	25286	41674	26
transferFrom(address,address,uint256)(bool)	20229	32943	36886	41674	82

[EdNoepel]

Per today's meeting, we do not wish to introduce this change at the current point of development.