PerpV2 module fixes and optimizations #192
Conversation
| // PerpV2 contract which provides a getter for baseToken UniswapV3 pools | ||
| IMarketRegistry public immutable perpMarketRegistry; | ||
|
|
||
| // Max perpetual positions per SetToken |
There was a problem hiding this comment.
Provide some context on why this is necessary
| onlyManagerAndValidSet(_setToken) | ||
| { | ||
| ActionInfo memory actionInfo = _createAndValidateActionInfo( | ||
| _validateTrade(_baseToken, _baseQuantityUnits); |
There was a problem hiding this comment.
Why get rid of createAndValidateActionInfo? Idk what the gas optimization is but unless it's significant I'd prefer the more readable code structure
| ActionInfo memory actionInfo = _createAndValidateActionInfo( | ||
| _validateTrade(_baseToken, _baseQuantityUnits); | ||
|
|
||
| int256 baseBalance = perpAccountBalance.getBase(address(_setToken), _baseToken); |
There was a problem hiding this comment.
It'd be nice to put lines 327-333 into it's own function. Perhaps you can pass in the totalSupply so that it doesn't have to get queried twice.
There was a problem hiding this comment.
Done in bde695. Basically brought back the createAndValidateActionInfo function and added those lines to that function.
| positions[_setToken].removeStorage(_baseToken); | ||
| } | ||
| } else { | ||
| require(positions[_setToken].length < maxPerpPositionsPerSet, "Exceeds max perpetual positions per set"); |
There was a problem hiding this comment.
Ideally we could check this before executing the trade (since this is only called in trade). Is there a way that we can determine the hasBaseToken variable earlier for a validation and then pass it down to the _updatePositionList function?
There was a problem hiding this comment.
That would decrease "separation of concerns" between the external and internal function. Also, it is on L2, where computation is cheap, so it shouldn't be a major problem.
| }); | ||
| }); | ||
|
|
||
| // todo: a test case which tries to flip the position? |
There was a problem hiding this comment.
I think we need a test for closing the position. We should check to make sure that when the exact negation of a baseToken position unit is passed in that it will completely zero out our vAsset position
There was a problem hiding this comment.
Tests for closing a position already exist. Added checks to ensure the base balance is zero to those tests in 597a16
0xSachinK
force-pushed
the
sachin/perpv2-module-bug-fixes
branch
from
6df45c4 to
7019cfe
Compare
This implementation also prevents the user from entering into a state where the module treats the base token position as closed while perp still considers it to be open because the module didn't close the entire position and a small non-negligible dust amount exists in perp protocol
0xSachinK
force-pushed
the
sachin/perpv2-module-bug-fixes
branch
from
4efcabf to
6a46c8a
Compare
No description provided.