-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit number of positions and collaterals per account #1774
Changes from 1 commit
db35a3a
91efb8b
214798a
79e4476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,7 @@ contract AsyncOrderSettlementModule is IAsyncOrderSettlementModule, IMarketEvent | |
runtime.marketId = asyncOrder.request.marketId; | ||
// check if account is flagged | ||
GlobalPerpsMarket.load().checkLiquidation(runtime.accountId); | ||
|
||
Position.Data storage oldPosition; | ||
(runtime.newPosition, runtime.totalFees, runtime.fillPrice, oldPosition) = asyncOrder | ||
.validateRequest(settlementStrategy, price); | ||
|
@@ -140,6 +141,11 @@ contract AsyncOrderSettlementModule is IAsyncOrderSettlementModule, IMarketEvent | |
PerpsMarketFactory.Data storage factory = PerpsMarketFactory.load(); | ||
PerpsAccount.Data storage perpsAccount = PerpsAccount.load(runtime.accountId); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes... I was wondering how that can happen, and keep it just in case. But, yeah, I'll get rid of it. |
||
|
||
// use fill price to calculate realized pnl | ||
(runtime.pnl, , , runtime.accruedFunding, ) = oldPosition.getPnl(runtime.fillPrice); | ||
runtime.pnlUint = MathUtil.abs(runtime.pnl); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,11 @@ contract PerpsAccountModule is IPerpsAccountModule { | |
PerpsAccount.Data storage account = PerpsAccount.create(accountId); | ||
uint128 perpsMarketId = perpsMarketFactory.perpsMarketId; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, move the if condition into the validation |
||
|
||
AsyncOrder.checkPendingOrder(account.id); | ||
|
||
if (amountDelta > 0) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,10 @@ library PerpsAccount { | |
|
||
error AccountLiquidatable(uint128 accountId); | ||
|
||
error MaxPositionsPerAccountReached(uint128 maxPositionsPerAccount); | ||
|
||
error MaxCollateralsPerAccountReached(uint128 maxCollateralsPerAccount); | ||
|
||
function load(uint128 id) internal pure returns (Data storage account) { | ||
bytes32 s = keccak256(abi.encode("io.synthetix.perps-market.Account", id)); | ||
|
||
|
@@ -69,6 +73,24 @@ library PerpsAccount { | |
} | ||
} | ||
|
||
function canOpenNewPosition(Data storage self) internal view { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would rename this to |
||
uint128 maxPositionsPerAccount = GlobalPerpsMarketConfiguration | ||
.load() | ||
.maxPositionsPerAccount; | ||
if (maxPositionsPerAccount <= self.openPositionMarketIds.length()) { | ||
revert MaxPositionsPerAccountReached(maxPositionsPerAccount); | ||
} | ||
} | ||
|
||
function canAddNewCollateral(Data storage self) internal view { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, maybe |
||
uint128 maxCollateralsPerAccount = GlobalPerpsMarketConfiguration | ||
.load() | ||
.maxCollateralsPerAccount; | ||
if (maxCollateralsPerAccount <= self.activeCollateralTypes.length()) { | ||
revert MaxCollateralsPerAccountReached(maxCollateralsPerAccount); | ||
} | ||
} | ||
|
||
function isEligibleForLiquidation( | ||
Data storage self | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
import { bn, bootstrapMarkets } from '../bootstrap'; | ||
import assertRevert from '@synthetixio/core-utils/utils/assertions/assert-revert'; | ||
import { BigNumber, ethers } from 'ethers'; | ||
import { snapshotCheckpoint } from '@synthetixio/core-utils/utils/mocha/snapshot'; | ||
|
||
describe('Markets - Max Collaterals per account', () => { | ||
const traderAccountIds = [2]; | ||
const _MARKET_PRICE = bn(100); | ||
const _UNLIMMITED = bn(100); | ||
const { systems, synthMarkets, provider, trader1, owner } = bootstrapMarkets({ | ||
synthMarkets: [ | ||
{ | ||
name: 'Collateral1', | ||
token: 'snxCL1', | ||
buyPrice: _MARKET_PRICE, | ||
sellPrice: _MARKET_PRICE, | ||
}, | ||
{ | ||
name: 'Collateral2', | ||
token: 'snxCL2', | ||
buyPrice: _MARKET_PRICE, | ||
sellPrice: _MARKET_PRICE, | ||
}, | ||
], | ||
perpsMarkets: [ | ||
{ | ||
requestedMarketId: bn(25), | ||
name: 'Market1', | ||
token: 'snxMK1', | ||
price: _MARKET_PRICE, | ||
lockedOiRatioD18: bn(0.01), | ||
}, | ||
], | ||
traderAccountIds, | ||
}); | ||
|
||
let market1Id: BigNumber, market2Id: BigNumber; | ||
before('identify actors', async () => { | ||
market1Id = synthMarkets()[0].marketId(); | ||
market2Id = synthMarkets()[1].marketId(); | ||
}); | ||
|
||
before('ensure account has enough balance of synths and market is approved', async () => { | ||
const usdAmount = _MARKET_PRICE.mul(100); | ||
const minAmountReceived = bn(100); | ||
const referrer = ethers.constants.AddressZero; | ||
await systems() | ||
.SpotMarket.connect(trader1()) | ||
.buy(market1Id, usdAmount, minAmountReceived, referrer); | ||
await synthMarkets()[0] | ||
.synth() | ||
.connect(trader1()) | ||
.approve(systems().PerpsMarket.address, _UNLIMMITED); | ||
|
||
await systems() | ||
.SpotMarket.connect(trader1()) | ||
.buy(market2Id, usdAmount, minAmountReceived, referrer); | ||
await synthMarkets()[1] | ||
.synth() | ||
.connect(trader1()) | ||
.approve(systems().PerpsMarket.address, _UNLIMMITED); | ||
}); | ||
|
||
before('ensure max collaterals is set to 0', async () => { | ||
await systems().PerpsMarket.connect(owner()).setMaxPerAccount(_UNLIMMITED, 0); | ||
}); | ||
|
||
const restore = snapshotCheckpoint(provider); | ||
|
||
it('Collaterals: reverts if attempting to add collateral and is set to zero', async () => { | ||
await assertRevert( | ||
systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(10)), | ||
'MaxCollateralsPerAccountReached("0")' | ||
); | ||
}); | ||
|
||
describe('Collaterals: when max collaterals per account is 1', () => { | ||
before(restore); | ||
|
||
before('set max collaterals per account', async () => { | ||
await systems().PerpsMarket.connect(owner()).setMaxPerAccount(_UNLIMMITED, 1); | ||
}); | ||
|
||
it('should be able to add collateral', async () => { | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(10)); | ||
}); | ||
|
||
it('should revert when attempting to add a 2nd collateral', async () => { | ||
await assertRevert( | ||
systems().PerpsMarket.connect(trader1()).modifyCollateral(2, market1Id, bn(10)), | ||
'MaxCollateralsPerAccountReached("1")' | ||
); | ||
}); | ||
|
||
it('can increase and decrease margin size for first collateral', async () => { | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(+1)); | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(-1)); | ||
}); | ||
}); | ||
|
||
describe('Collaterals: when max collaterals per account is unlimmited', () => { | ||
before(restore); | ||
|
||
before('set max collaterals per account', async () => { | ||
await systems().PerpsMarket.connect(owner()).setMaxPerAccount(_UNLIMMITED, _UNLIMMITED); | ||
}); | ||
|
||
it('should be able to add more than one collateral type (two)', async () => { | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(10)); | ||
|
||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, market1Id, bn(10)); | ||
}); | ||
|
||
describe('when reducing the max collaterals per account', () => { | ||
before('reduce max collaterals per account', async () => { | ||
await systems().PerpsMarket.connect(owner()).setMaxPerAccount(_UNLIMMITED, 2); | ||
}); | ||
|
||
it('should revert when attempting to add a 3rd collateral', async () => { | ||
await assertRevert( | ||
systems().PerpsMarket.connect(trader1()).modifyCollateral(2, market2Id, bn(10)), | ||
'MaxCollateralsPerAccountReached("2")' | ||
); | ||
}); | ||
|
||
it('should allow a new collateral if another is depleted', async () => { | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, 0, bn(-10)); | ||
await systems().PerpsMarket.connect(trader1()).modifyCollateral(2, market2Id, bn(10)); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would move the if condition into the validation itself. maybe something like this:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍🏽