Perps changes batch 1 #1780
Perps changes batch 1 #1780
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1780 +/- ##
===========================================
- Coverage 94.99% 94.98% -0.02%
===========================================
Files 105 105
Lines 2617 2611 -6
Branches 791 788 -3
===========================================
- Hits 2486 2480 -6
Misses 131 131
Continue to review full report at Codecov.
|
artdgn
force-pushed
the
perps-changes-batch1
branch
from
0bc6af7
to
00ab6ba
Compare
artdgn
force-pushed
the
perps-changes-batch1
branch
from
00ab6ba
to
cf0d87a
Compare
|
btw, fork-tests are failing because APE & DYDX markets are paused although according to config they shouldn't be, and there's no automated choice for that situation in the deploy script (it asks for a decision). |
| @@ -82,8 +70,8 @@ contract PerpsV2Settings is Owned, MixinPerpsV2MarketSettings, IPerpsV2Settings | |||
| /* | |||
| * The maximum allowable notional value on each side of a market. | |||
| */ | |||
| function maxMarketValueUSD(bytes32 _marketKey) public view returns (uint) { | |||
| return _maxMarketValueUSD(_marketKey); | |||
| function maxSingleSideValueUSD(bytes32 _marketKey) public view returns (uint) { | |||
There was a problem hiding this comment.
could be declared as external to save some gas
There was a problem hiding this comment.
yeah good point, not sure why these views are public
| require(_takerFeeNextPrice <= 1e18, "taker fee greater than 1"); | ||
| _setParameter(_marketKey, PARAMETER_TAKER_FEE_NEXT_PRICE, _takerFeeNextPrice); | ||
| function setBaseFee(bytes32 _marketKey, uint _baseFee) public onlyOwner { | ||
| require(_baseFee <= 1e18, "taker fee greater than 1"); |
There was a problem hiding this comment.
[np] maybe this should be "base fee greater than 1"
There was a problem hiding this comment.
I get that technically this is the "taker", but I had to look that up 😅
It might be better to just call it exactly what the setting name is just to be more explicit.
There was a problem hiding this comment.
yep, missed when renaming stuff
| require(_makerFeeNextPrice <= 1e18, "maker fee greater than 1"); | ||
| _setParameter(_marketKey, PARAMETER_MAKER_FEE_NEXT_PRICE, _makerFeeNextPrice); | ||
| function setBaseFeeNextPrice(bytes32 _marketKey, uint _baseFeeNextPrice) public onlyOwner { | ||
| require(_baseFeeNextPrice <= 1e18, "taker fee greater than 1"); |
| * Allows markets to issue exchange fees into the fee pool and notify it that this occurred. | ||
| * This function is not callable through the proxy, only underlying contracts interact; | ||
| * it reverts if not called by a known market. | ||
| */ | ||
| function payFee(uint amount, bytes32 trackingCode) external onlyMarkets { |
There was a problem hiding this comment.
perhaps rename this to something else in order to differentiate it from the futures v1 payFee function below.
e.g. payFeeWithTracking?
There was a problem hiding this comment.
I think the v1 method will be removed with v1 stuff after the migration, so I think it makes more sense to have the default method called just payFee and always expect a tracking code as part of its interface (and it's up to the caller to pass a 0).
| // check that market isn't suspended, revert with appropriate message | ||
| _systemStatus().requireFuturesMarketActive(marketKey); // asset and market may be different | ||
| if (allowMarketPaused) { | ||
| // this will check system activbe, exchange active, futures active |
| @@ -80,19 +80,24 @@ contract PerpsV2MarketBase is MixinPerpsV2MarketSettings, IPerpsV2BaseTypes { | |||
| */ | |||
| mapping(address => Position) public positions; | |||
|
|
|||
| /// mapping of position id to account addresses | |||
| mapping(uint => address) public positionIdOwner; | |||
There was a problem hiding this comment.
[np] "owner" is kind of a sensitive word... could we make it something like accountForPositionId
Some small changes to the contracts: