position descriptor proxy tests #427
Conversation
| } | ||
|
|
||
| /// @notice Returns the native currency label as a string | ||
| function nativeCurrencyLabel() public view returns (string memory) { |
There was a problem hiding this comment.
would love to see unit tests for this function
There was a problem hiding this comment.
code copied from here btw, but I couldn't find a unit test on the fly there either:
https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/NonfungibleTokenPositionDescriptor.sol#L35-L45
src/PositionDescriptor.sol
Outdated
| @@ -26,14 +26,27 @@ contract PositionDescriptor is IPositionDescriptor { | |||
| address private constant WBTC = 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599; | |||
|
|
|||
| address public immutable wrappedNative; | |||
| string public nativeCurrencyLabel; | |||
| bytes32 public immutable nativeCurrencyLabelBytes; | |||
There was a problem hiding this comment.
I suggest making this variable private as we already have a getter
| bytes32 public immutable nativeCurrencyLabelBytes; | |
| bytes32 private immutable nativeCurrencyLabelBytes; |
| } | ||
|
|
||
| /// @notice Returns the native currency label as a string | ||
| function nativeCurrencyLabel() public view returns (string memory) { |
There was a problem hiding this comment.
code copied from here btw, but I couldn't find a unit test on the fly there either:
https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/NonfungibleTokenPositionDescriptor.sol#L35-L45
| assertEq(proxyAsImplementation.nativeCurrencyLabelBytes(), nativeCurrencyLabelBytes); | ||
| } | ||
|
|
||
| function test_nativeCurrencyLabel_succeeds() public { |
There was a problem hiding this comment.
i added the unit test here!
There was a problem hiding this comment.
update test to also test the raw bytes32
There was a problem hiding this comment.
i cant since its private
There was a problem hiding this comment.
unless we change to public
| ? tickUpper <= TickMath.getTickAtSqrtPrice(sqrtPriceX96) | ||
| : tickLower > TickMath.getTickAtSqrtPrice(sqrtPriceX96) | ||
| ? TickMath.getSqrtPriceAtTick(tickUpper) <= sqrtPriceX96 | ||
| : TickMath.getSqrtPriceAtTick(tickLower) >= sqrtPriceX96 |
There was a problem hiding this comment.
YES so much better thank you
There was a problem hiding this comment.
with this i can sleep happy 😂
| @@ -26,14 +26,27 @@ contract PositionDescriptor is IPositionDescriptor { | |||
| address private constant WBTC = 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599; | |||
|
|
|||
| address public immutable wrappedNative; | |||
| string public nativeCurrencyLabel; | |||
| bytes32 private immutable nativeCurrencyLabelBytes; | |||
There was a problem hiding this comment.
daniel suggested we make it private since we already have a public getter for nativeCurrencyLabel
There was a problem hiding this comment.
i dont see why it can't just be public tho.. theres no downside
| assertEq(proxyAsImplementation.nativeCurrencyLabelBytes(), nativeCurrencyLabelBytes); | ||
| } | ||
|
|
||
| function test_nativeCurrencyLabel_succeeds() public { |
There was a problem hiding this comment.
update test to also test the raw bytes32
| @@ -918,15 +918,15 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF | |||
| // Read below as: | |||
| // bool currency0IsFOT = fotKey.currency0 == Currency.wrap(address(fotToken)); | |||
| // bool positionIsEntirelyInOtherToken = currency0IsFOT | |||
| // ? tickUpper <= TickMath.getTickAtSqrtPrice(sqrtPriceX96) | |||
| // : tickLower > TickMath.getTickAtSqrtPrice(sqrtPriceX96); | |||
| // ? TickMath.getSqrtPriceAtTick(tickUpper) <= sqrtPriceX96 | |||
test/shared/PosmTestSetup.sol
Outdated
| ); | ||
| } | ||
|
|
||
| function deployDescriptor(IPoolManager poolManager, bytes32 label) internal returns (IPositionDescriptor) { | ||
| positionDescriptor = | ||
| Deploy.positionDescriptor(address(poolManager), 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2, label, hex"00"); |
There was a problem hiding this comment.
why is that address hardcoded for native? shouldn't it be address(_WETH9)?
There was a problem hiding this comment.
its hardcoded so i can check if its equal in PositionDescriptor.t.sol
if it wasnt hardcoded, foundry would deploy WETH9 to a random address
There was a problem hiding this comment.
could do it the opposite way
There was a problem hiding this comment.
its just that if we want to run fork tests this would break no?
There was a problem hiding this comment.
hm i guess not if we're not doing any actual transfers with that token, it is odd though that the weth we pass into posm will be different than the weth in position descriptor
| @@ -1113,7 +1099,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF | |||
|
|
|||
| // After redeeming the position, the liquidity provider should not have received any of the fot token since the position was entirely in the other token. | |||
| assertEq(fotKey.currency1.balanceOf(address(this)), balance1._after); | |||
| assertGe(fotKey.currency0.balanceOf(address(this)), balance0._after); | |||
| assertGt(fotKey.currency0.balanceOf(address(this)), balance0._after); | |||
There was a problem hiding this comment.
this assumes that the fot token is currency1?
Related Issue
testing position descriptor as proxy
Description of changes