Limit number of positions and collaterals per account #1774
Conversation
| if (oldPosition.size == 0) { | ||
| //is a new position, check if not exceeding max positions per account | ||
| perpsAccount.canOpenNewPosition(); | ||
| } |
There was a problem hiding this comment.
i dont think you need this validation in settle. since you can't have more than one order at a time, i don't see a scenario where validation passes commitment then fails here.
There was a problem hiding this comment.
Yes... I was wondering how that can happen, and keep it just in case. But, yeah, I'll get rid of it.
| if (PerpsMarket.accountPosition(commitment.marketId, commitment.accountId).size == 0) { | ||
| //is a new position, check if not exceeding max positions per account | ||
| PerpsAccount.load(commitment.accountId).canOpenNewPosition(); | ||
| } |
There was a problem hiding this comment.
i would move the if condition into the validation itself. maybe something like this:
PerpsAccount.validateMaxPositions(commitment.marketId, commitment.accountId);
and load the position and account in the validation itself. just cleans up the modules a bit more.
There was a problem hiding this comment.
wish we still had createValid, then this call for validating max positions can just live there.
i have some ideas on this, but can leave it to a separate PR 👍🏽
| if (account.collateralAmounts[synthMarketId] == 0) { | ||
| // if the account has no collateral in this market, we need to check if it can add a new collateral | ||
| account.canAddNewCollateral(); | ||
| } |
There was a problem hiding this comment.
same as above, move the if condition into the validation
| @@ -69,6 +73,24 @@ library PerpsAccount { | |||
| } | |||
| } | |||
|
|
|||
| function canOpenNewPosition(Data storage self) internal view { | |||
There was a problem hiding this comment.
would rename this to validateMaxPositions. can implies it will return a boolean.
| } | ||
| } | ||
|
|
||
| function canAddNewCollateral(Data storage self) internal view { |
There was a problem hiding this comment.
same as above, maybe validateMaxCollaterals
| ) internal { | ||
| checkPendingOrder(newRequest.accountId); | ||
|
|
||
| PerpsAccount.load(newRequest.accountId).validateMaxPositions(newRequest.marketId); |
There was a problem hiding this comment.
can just do the account loading in the validateMaxPositions function.
so could just do PerpsAccount.validateMaxPositions(newRequest.accountId, newRequest.marketId);
non blocking
There was a problem hiding this comment.
good call, will update
| uint128 maxPositionsPerAccount = GlobalPerpsMarketConfiguration | ||
| .load() | ||
| .maxPositionsPerAccount; | ||
| if (maxPositionsPerAccount <= self.openPositionMarketIds.length()) { |
There was a problem hiding this comment.
why <= instead of just <?
if max is set to 5, can't you have 5 open positions?
There was a problem hiding this comment.
because we check here if we can add another => in your example we'll check when adding the 5th so we need to have 4 already there at most
No description provided.