nexlabs22 / nex-dex-contracts Goto Github PK

Should adding and removing users be a public function?

I see you changed a bunch of code from uint to int to account for the fact that variables can have negative values.
However, in line 650 and 651:
uint newMargin = 100*uint(accountValue)/newPositionNotional;
if (newMargin > 0 && newMargin <= saveLevelMargin) {

You check whether newMargin is >0 when its a unsigned, so it can never be negative there right? If I understand correctly than newMargin should be an int not uint.

652 if (newMargin > 0 && newMargin <= saveLevelMargin) {
newMargin !=0

typo

I suggest to not use the oracle to adjust the virtual pool price.
An example, say we have only one user, Bob, he buys and the price goes up, then he sells and the price moves down back to where it started.
He cannot make a profit but he can open a position, This is important because somebody needs to be the first to open a position.
Only when the pool price moves outside of bobs own trading actions can he make a profit or loss. (f.e. because of Alice buys after Bob)

If we would adjust the pool price according to the oracle price, Bob can make a profit that is not covered by other people their losses. We do not take on exposure to people and their trades, so this is impossible.

My solution: the collateral balance of the long and short holders gets exposed to these price changes. If bob is the only long, he gets money from nobody (cause there are no shorters) and thus he gets nothing. The price of the pool is not touched.

If we change the collateral with the (unrealized) pnl, we need to set the pnl to zero. (unless we close the position and throw away all the storage that comes with it?)

Can you change this for every instance where the collateral is changed with the pnl?

Can you track the totalInvestedValue and totalAssetSize after each moment of change (trade/liquidation etc.)? -> usually where you currently track PNL.

the vUSD of the bored ape is positive, thus the vUSD value the user was holding is negative
so f.e. 4000 + (-3000) = 1000

Right now it is possible to liquidate yourself, the liquidation then takes priority and the original transaction still follows after.
This causes a double transactions and the balance to go in the opposite way.

Perhaps you can add the exclusion that a user cannot liquidate themselves, instead they will just be able to do their transaction as if normal.

Can you charge the trading fee on the vUsd size from the users collateral to an address controlled by NexLabs (f.e. the onlyOwner address, but let me know what you thing is smart) for every trade made (except perhaps liquidation sale)

You can replace below quotes, I have improved them because a user might be able to read them I assume.
If you would like help with this, I can recommend Grammarly, it is smarter than for example google docs.

"You should wait at least 12 hours after latest update" -> "You should wait at least 12 hours after the latest update."
"newFee should be between 1 to 500" -> "The newFee should be between 1 and 500 (0.01% - 5%)."

"You cannot withdraw because your margin rate is the lower than saveMargin level" -> "
"You cannot withdraw because your margin is lower than the saveMargin level."

"Desire amount is more than collateral balance" ->
"Requested withdrawal amount is larger than the collateral balance."

"out of vitrual collateral balance" ->
"Requested amount is larger than the virtual collateral balance."

"You can't move the price more than 10% far from the oracle price" ->
"You can't move the price more than 10% away from the oracle price."

"You can't open this position because you probability will be liquidated by this margin" ->
"Insufficient margin to open position with requested size."

"You dont have enough asset size to close the long position" ->
"Reduce only order can only close long size equal or less than the outstanding asset size."

"You dont have enough asset size to close the short position" ->
"Reduce only order can only close short size equal or less than the outstanding asset size."

"You dont have enough asset size to close the all positions" ->
"Reduce only order can only close size equal or less than the outstanding asset size."

2x line 766 same:
"user can not be liquidate" ->
"User is not hard liquidatable."

"user can not be partially liquidated" ->
"User is not partially liquidatable."

"greater than insurance funds balance" ->
"Requested collect amount is larger than the ContractFee balance."

should be + no?
In that case no if statement needed perhaps

Currently there are no constraints to a change in the oracle price.
I would like a constraint MaxOracleChange from the previous price and minimal interval 1 hour.

MaxOracleChange = 40%
MinOracleInterval = 1 hour
PreviousOraclePrice = xxx
Blockheight/TimestampPreviousOraclePrice = xxx

OracleChange = positive((newOraclePrice -oldOraclePrice)/ oldOraclePrice * 100)
if (OracleChange > MaxOracleChange & someWayToSaveleyCheckTheTimeIsMoreThanSay40Min ) {
if (newOraclePrice < oldOraclePrice){
newOraclePrice = oldOraclePrice * ((100-MaxOracleChange)/100)
} else {
newOraclePrice = oldOraclePrice * ((100+MaxOracleChange)/100)
}
}

I would also like to consider a minimum deposit of say 5 usdc and a minimum position of also say 5 usdc to ensure that nobody can clog the smart contract without active addresses that need to be checked for liquidation all the time.

uint256 fundingFee = assetSize * (indexPrice - oraclePrice);
-> uint256 fundingFee = assetSize * (indexPrice - oraclePrice) * (1/24)
Every hour I would like 1/24 of the difference to be adjusted.
It is to limit the amount of funding payment that a thrown back and forward between users.
Every hour 1/24 of the difference is adjusted, so after a day of funding rates the Nexlabs index price approaches the oracle price.

Just wondering why I have two supply the address twice

We should probably try to liquidate people that are hard liquidable first over people that are only partially liquidable.
I see 4 instances: open long/short and close long/short.

Knowing that a liquidation can trigger another due to the price change.
Is it true that we cannot loop (in a while >0 liquidatable, go through that list) over the liquidations within are single transaction as that would possibly blow up a transactions gas cost.
If we could, than we can check for all partial liquidations first, continue looping until we had all of them, and then check for all hard liquidations and continue looping until we have all of them.

function partialLiquidate and partialLiquidateUsers have both an internal and a public.
When we check in each step of every trade for liquidations public does not need to be able to liquidate users anymore afaik so it can be removed, or did you want to check how the contract was working?

//withdraw collateral
function withdrawEther(uint256 _amount) public {
uint256 userMargin = getUserMargin(msg.sender);
uint256 totalPositionNotional = totalPositionNotional(msg.sender);
require(
totalPositionNotional == 0 || userMargin > 60,
"You cannot withdraw because your margin rate is the lower than saveMargin level"
);
require(
collateral[ETHER][msg.sender] >= _amount,
"Desire amount is more than collateral balance"
);
collateral[ETHER][msg.sender] = collateral[ETHER][msg.sender].sub(_amount);
payable(msg.sender).transfer(_amount);
emit Withdraw(ETHER, msg.sender, _amount, collateral[ETHER][msg.sender]);
}

I would like that the maximum withdrawable amount is until userMargin minimal is 60 if there is a positionNotional. (if they have no position than ofcourse they can go to zero)

Probably all instance of:
Different -> Difference

I got trouble depositing collateral
The price is 0.42 goerli ETH
Is this network congestion or because our smart contract is very expensive to make any change?

(line 500)
uint256 realizeAmout = (liquidateAmount * discountRate) / 100;
the brackets serve no purpose
->
uint256 realizeAmout = liquidateAmount * discountRate / 100;

if you want to show where the brackets originally came from you can use:
uint256 realizeAmout = liquidateAmount * (discountRate / 100);

Please set the funding fee relative to the smallest size of:
x = min(allLongvBaycBalance, -1* allShortBaycBalance)
uint256 fundingFee = x * (indexPrice - oraclePrice)) / 24

So that the funding fee of long and short holders are net zero.
Otherwise you can create collateral (real money) out nothing.

Example fundingFee =50$
Short position size 1000 (1 user), Long 10000 (2 users one, 2k one 8k)
Short pays 50 * 1k/1k, first long user 2k/10K * 50 and second long user 8k/10k * 50.

https://goerli.etherscan.io/address/0xd43B5F0cBdE76D1953FA05AE82080bb53993dd89#code
The contract mentiones compiler bugs, is this an issue?

Compiler specific version warnings:

The compiled contract might be susceptible to StorageWriteRemovalBeforeConditionalTermination (medium/high-severity), AbiReencodingHeadOverflowWithStaticArrayCleanup (medium-severity), DirtyBytesArrayToStorage (low-severity), InlineAssemblyMemorySideEffects (medium-severity), DataLocationChangeInInternalOverride (very low-severity), NestedCalldataArrayAbiReencodingSizeValidation (very low-severity) Solidity Compiler Bugs.

isUserExist
->
doesUserExist

Is there a way to be more descriptive in the error messages from the code?

Can you explain this formula with an example for me?

We might be able to save gas costs by storing these in uint128 or even smaller and grouping them together.
Then if the oracle variable/collateral comes in as a 10^18 or for example the stablecoin then we need to divide those to get the correct amount, but this division might be cheaper than the memory access.

https://techblog.geekyants.com/tips-and-tricks-in-solidity

Ill leave this thought open in case you agree/disagree with the idea to not have an insurance fund.
Then we would be better of renaming this, since it leads to confusion with anybody who reads the contract.

Also instead of removeInsuranceFunds I would like collectContractFee (so Collect instead of "remove")

//calculate liquidation amount to turn back the user margin to the save level(60%)
function calculatePartialLiquidateValue(address _user) public view returns (uint256 x) {
uint256 totalAccountValue = totalAccountValue(_user);
uint256 totalPositionNotional = totalPositionNotional(_user);
uint256 numerator = (totalPositionNotional * saveLevelMargin) / 100 - totalAccountValue;
uint256 denominator = saveLevelMargin / 100 - discountRate / 100;
x = numerator / denominator;
}

Numerator and denominator are incorrect as far as I can see from the updated excalidraw.

After I do a roundtrip 300 long and sell the 300 long position after I remain with
99400000000000085696
After a single trade my collateral also was not exactly 997 but slightly more.

So I assume the 85696 is a rounding error? Perhaps there is way to remove it.

This solution seems to directly adjust users their collateral.
My suggestion was to make a list of users virtual collateral in the main contract and
sum: collateral = realcollateral + virtualcolleral,
whenever a user wants to change their position.
Is my suggestion viable at all?

Do you think your solution or this solution is can be optimized?

Example:
Oracle price is 100ETH/per APE right
So 1000USDC/100APES = 10$ per APE

Flip the nominator with the denominator? for currentPrice & newPrice

Also here I think should be + sign and no need for if statement similar to previous issue.

Here we are testing whether the user itself is partially or completely liquidatable. Right now this function is excess since, with the newly added function to stay above 60%, the user will never be liquidatable, so we do not need to check for that, we just need to run the partially and fully liquidate functions for all the other users without a condition IMO.

A users margin can go below zero, if people before them get liquidated in the same transaction, these people currently do not get liquidated. I would like these people also to get liquidated and I would like the virtualCollateral balance of users who profit from this liquidation to be lowered with the negative value, with the lowering amount/value relative to the weight of their position size.
This ensures there is never any bad debt

couple of "]" in this function that seems to not close a [

The 10.GetAllActiveUsers list is a list that keeps expanding, instead of removing it changes an address into 0x000... (so empty)
it seems. So the list will get longer.

line 132: delete activeUsers[i];

line 261
function openLongOrderUsd()
This function adds to the TotalInvestedValue

But if a user closes their order before the order is filled, or upon only partial fill. Is the TotalInvestedValue reduced?

function getLongUsdAmountOut(uint256 _vBaycAmount) public view returns (uint256) {
}
function getShortUsdAmountOut(uint256 _vBaycAmount) public view returns (uint256)
}

I prefer getLongVusdAmountOut, getShortVusdAmountOut

Do you agree that liquidating users every time somebody wants to trade is a viable idea?

When I open a position, I can immediately be at risk of being liquidated because I can open a position larger than the allowed margin.
For now during the testing, you can actually try to liquidate me and see how that works.
0x53FebBeE060F68CCE33Bd7785f352Ca3c7A205BB
But after I would like that to change.

function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
}

Is it possible to renounce the ownership and still perform vital operations such as provide oracle price, liquidate and receive protocol fees?

realizeVitrualCollAmount -> realizeVirtualCollAmount

uint256 highestLongOrderPrice = longOrders[longOrders.length - 1].price;
uint256 lowestShortPrice = shortOrders[shortOrders.length - 1].price;

please rename either one of them, f.e. lowestShortPrice = lowestShortOrderPrice
for consistency in nomenclature

line 617 if (accountValue > 0 && positionNotional > 0) {

Count you merge the two into?:
if ((accountValue > 0 || accountValue <0) && positionNotional > 0) {

Even better maybe, something that you do later: != 0

Can you put (some of the remaining) margin variables in the code interactively?
Thx.

Is it only possible to call the initialVirtualPool function once, at creation?

If the owner is compromised, they can change the amounts in the pool, open a position in a certain direction and essentially drain the pool by giving themselves a favourable price after they already opened a position.

uint256 indexPrice = 0;
if (lowestShortPrice < highestLongOrderPrice) {
indexPrice = (highestLongOrderPrice - lowestShortPrice) / 2 + lowestShortPrice;
}
if (lowestShortPrice > highestLongOrderPrice) {
indexPrice = (lowestShortPrice - highestLongOrderPrice) / 2 + highestLongOrderPrice;
}
return indexPrice;
}

-> simpler formula no if condition required:
(highestLongOrderPrice + lowestShortOrderPrice) / 2
example (6+8)/2 = 7 or (8+6)/2 = 7 regardless.

Perhaps add a condition so that we cannot change the fee to fast and put it too high.
Lets say max 5%, min 0.01%, 12hr timelock might be interesting etc, maybe max change from previous number by say absolute 1%.

I we could change this too fast, we could hold user collateral hostage, this would be a problem if the onlyOwner address get hacked.

//Total of user long and short positions
function totalPositionNotional(address _user) public view returns (uint256) {
uint256 latestPrice = nftOracle.showPrice(latestRequestId);
uint256 totalPositionsvalue = 0;
for (uint256 i = 0; i < positions.length; i++) {
if (positions[i].longAddress == _user) {
totalPositionsvalue += positions[i].positionSize * latestPrice;
}
if (positions[i].shortAddress == _user) {
totalPositionsvalue += positions[i].positionSize * latestPrice;
}
}
return totalPositionsvalue;
}

Why is there an if condition if the formula:
totalPositionsvalue += positions[i].positionSize * latestPrice;
is the same