Fix open/close position accounting

[adrena-orex]

Hello, this PR provide a solution to the following issues:

Issue 1

The function add_position lockup tokens on the custody:

        stats.locked_amount = math::checked_add(stats.locked_amount, position.locked_amount)?;

https://github.com/solana-labs/perpetuals/blob/eb9fe0a6f962665faf736ee068c4d77fd75c825c/programs/perpetuals/src/state/custody.rs#L463C1-L463C95

The problem is that when it's a short position, the locked_amount is in collateral unit.

https://github.com/solana-labs/perpetuals/blob/eb9fe0a6f962665faf736ee068c4d77fd75c825c/programs/perpetuals/src/instructions/open_position.rs#L215C2-L215C2

Example: with maxpayoff at 100%, when shorting 1 ETH. Today we're locking up 1500 ETH from the ETH custody (instead of 1 ETH).

---

Issue 2

The checks on max_position_locked_usd and max_total_locked_usd should be made on the locked token - which is the collateral custody in case of short.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
perpetuals	❌ Failed (Inspect)			Oct 18, 2023 11:29am

[adrena-orex]

Wanna make sure we have the same understanding @askibin :

• When the user long ETH, we lock-up ETH up to the maxpayoff of the position and the user pay borrow fees on the locked ETH.

• When the user short ETH with USDC, we lock-up USDC up to the maxpayoff of the position and the user pay borrow fees on the locked USDC. We do not lock ETH.

[askibin]

I looked at the PR and I agree that locked amount is not updated properly in global position for shorts or virtual trades. Just two things to note:

1. in open/close position instruction you refactored some code leading to *custody = collateral_custody.clone(); potentially called twice. Do you know how expensive that copy is? Just trying to understand if shorter code worth it.
2. locked_amount is used to set upper limit for user's pnl. And PnL itself is calculated using the trade token custody only. While the collateral custody is used to calculate interest. You removed locked_amount update for the trade custody (for shorts/virtual) which might lead to an issue. So what if you properly calculate locked_amount for both custodies and update them in add/remove position?

[adrena-orex]

Thank you for your feedback 🙏 Let's dive on each comment:

I looked at the PR and I agree that locked amount is not updated properly in global position for shorts or virtual trades. Just two things to note:

1. in open/close position instruction you refactored some code leading to *custody = collateral_custody.clone(); potentially called twice. Do you know how expensive that copy is? Just trying to understand if shorter code worth it.
2. locked_amount is used to set upper limit for user's pnl. And PnL itself is calculated using the trade token custody only. While the collateral custody is used to calculate interest. You removed locked_amount update for the trade custody (for shorts/virtual) which might lead to an issue. So what if you properly calculate locked_amount for both custodies and update them in add/remove position?

1.

Used sol_log_compute_units() to display computing with old and refactored method from "Check position risks" to the end of the instruction:

refactored method

• Long position: consumed 18719 compute units
• Short position: consumed 16752 compute units

old method

• Long position: consumed 17735 compute units
• Short position: consumed 16753 compute units

Difference is 984 computing for opening a long position

Checked payer balance before and after, tx always cost 10k lamports. Not sure there the impact on the price in live conditions.

2.

a)

Would you have a specific area in the code for me to look at please regarding the upper limit and PnL please?

I've taken a look at get_pnl_usd and it looks like collateral_custody is used for calculations. Checked other use of locked_amount couldn't find.

https://github.com/solana-labs/perpetuals/blob/eb9fe0a6f962665faf736ee068c4d77fd75c825c/programs/perpetuals/src/state/pool.rs#L575

I'm probably missing something there ^^'

---

b)

Yes we could double lock the stable and the principal, but it would lead to 2 borrows to be paid correct? It looks like also the code is only handling interests over the collateral today? 🤔

[mzs-dev]

One potential solution that we have implemented is to make sure to have a single stable coin custody that acts as default collateral and lockup custody for shorts and virtual assets and then track the assets on collateral custody while tracking collective positions on the target custody so that both interest and collective pnl calculations are accurate

[askibin]

Would you have a specific area in the code for me to look at please regarding the upper limit and PnL please?

it is used here: https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L654 https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L698 collateral custody is the trading token custody for longs so locked_amount should be updated properly for the trading custody in the aggregated position

[Adrena-Corto]

So we agree on the first part that it should be USDC locked when shorted (to ensure that we can pay the user PnL).

About you comment

locked_amount is used to set upper limit for user's pnl. And PnL itself is calculated using the trade token custody only. While the collateral custody is used to calculate interest. You removed locked_amount update for the trade custody (for shorts/virtual) which might lead to an issue. So what if you properly calculate locked_amount for both custodies and update them in add/remove position?

As we understand it, it doesn't matter if we lock the trade_custody.

which might lead to an issue What issue might it lead to?

Let's say a user is shorting ETH. If we lock USDC for the payout + ETH for the "short", then we immobilise double liquidity, and we ensure the loss on the ETH if price ends up going up. I understand that logically the pool is the counterparty to the trade, but the pool doesn't need to actually be long (since it's a virtual short, we don't sell the ETH). Holding that ETH is a downside without any advantage as far as we understood. The only important bit is that the USDC are there for the payout, so shorts are available as long as there is USDC to borrow.

About your concerns regarding erroneous calculations, we did't find the relation in the code you shared since the collateral_custody is USDC? We don't see where the "locked_amount" is used, where does it have effect in the codebase? We don't find it. Maybe we could jump in a call to sort this out, we might misunderstand something.

One potential solution that we have implemented is to make sure to have a single stable coin custody that acts as default collateral and lockup custody for shorts and virtual assets and then track the assets on collateral custody while tracking collective positions on the target custody so that both interest and collective pnl calculations are accurate

@mzs-dev This sentence is beefy, we are not sure we follow 100%. Does that mean you guys lock both collateral (stable) and trade custody assets, and so if so, did you implement borrow fee payments on both assets for the duration of the short?

That reduce liquidity for very little advantage (for flash trade instead of serving 100 users and getting single interest on USDC form each of them, you'll serve 50 users that pay double interests. You'll loose fees on open/close positions though. Doesn't seems like a good thing for anyone)

[askibin]

I see where your frustration is coming from. There are two different notions of locked amounts. The one that you are referring to in your latest message (what gets locked for potential pnl payoff) happens here: https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/instructions/open_position.rs#L302 and already works as you described, i.e. tokens are only locked in the collateral custody, which is a stablecoin for shorts or virtual. The original locked_amount logic issue which you pointed out: https://github.com/solana-labs/perpetuals/blob/eb9fe0a6f962665faf736ee068c4d77fd75c825c/programs/perpetuals/src/state/custody.rs#L463C1-L463C95 is used for something else. Those stats are used to keep track of the global (aggregated) user position to determine unrealized pnl for all traders combined. This is the only place where aggregated positions are needed: https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L765 The locked amount in aggregated positions is not used to lock tokens for payouts, but rather to determine the upper limits of these payoffs (which is kinda the same, with the only difference is that there is no actual locking in custody, just a variable in position stats that we can access at the time of aggregated pnl calc). And it is used here: https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L654 https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L698

Now, back to the original issue. Aggregated positions should be tracked in trading coin custodies to ensure we can compute pnl correctly. If a trader shorts ETH we need to know what their position size is in ETH. If we update position stats in collateral custody only, we won't be able to compute the pnl. On the other hand, if we keep track of the aggregated position in the trading coin custody only, we won't be able to compute the interest properly for shorts (as it should be based on deposited stablecoin amount and not trade coin).

One potential solution that was discussed is to keep track of all trade coin positions in stablecoin custody (i.e. via array), which is inefficient. So instead, aggregated positions are tracked in both custodies. Trade token custody is used to compute pnl with zero interest, and stablecoin custody is used to compute interest only with zero pnl. Combined, they are supposed to produce the intended result. Check it out here https://github.com/solana-labs/perpetuals/blob/master/programs/perpetuals/src/state/pool.rs#L766 Since the global position in stablecoin custody is only used for interest calc, locked amount is not used there.

So again, stats for PNL tracking don't affect individual traders' positions and don't lock any funds. These are for aggregated pnl only. But I agree with you that the locked amount for position stats is not updated properly in the case of shorts, as there must be a trade coin amount and not stablecoin. This needs to be fixed.

[Adrena-Corto]

Ok thanks for the thorough explanation, that make sense now! No frustration here, but was puzzling to us

[adrena-orex]

Thank you @askibin for the detailled explanation of how this works. Makes me have a better understanding!

For the implementation, I'm interested to see what's you think could be a good alternative to arrays?

One potential solution that was discussed is to keep track of all trade coin positions in stablecoin custody (i.e. via array), which is inefficient.

Having an array such as [{ custody: Pubkey, locked_amount: u64 }] in stable custody stats could fix the problem by having exact accounting of possible payouts. Agreed that the size of the array depends on the amount of custodies which is not ideal.

So instead, aggregated positions are tracked in both custodies. Trade token custody is used to compute pnl with zero interest, and stablecoin custody is used to compute interest only with zero pnl. Combined, they are supposed to produce the intended result.

Not sure what info could be stored in the custody stats that would makes the calculation to be correct.

i.e: John opens a SHORT position for 1 ETH (with 1 ETH = $1,500), providing 500 USDC as collateral.

The locked_amount in collateral_custody is 1,490 (USDC). Equivalence is 0.99 ETH. If you store 0.99 ETH there, when calculating the PnL, if the price of ETH is $1,200, the calculated max_profit would be $1,188 which is incorrect. The position real max_profit is collateral_custody_price * collateral_custody.locked_amount. Example with USDC as $0.99, max_profit is $1,475.1.

What we could do is create max_short_payout_usd in custody stats. But we would have to consider 1 stable custody = 1 usd. Which ultimately leads to small imprecision in PnL calculation.

[mzs-dev]

One potential solution that we have implemented is to make sure to have a single stable coin custody that acts as default collateral and lockup custody for shorts and virtual assets and then track the assets on collateral custody while tracking collective positions on the target custody so that both interest and collective pnl calculations are accurate

@mzs-dev This sentence is beefy, we are not sure we follow 100%. Does that mean you guys lock both collateral (stable) and trade custody assets, and so if so, did you implement borrow fee payments on both assets for the duration of the short?

That reduce liquidity for very little advantage (for flash trade instead of serving 100 users and getting single interest on USDC form each of them, you'll serve 50 users that pay double interests. You'll loose fees on open/close positions though. Doesn't seems like a good thing for anyone)

Hope your misconception here is also cleared @Adrena-Corto. Extending on @askibin's explanation, we decided to limit stable coin custody to just one and make it the default collateral custody for the interim to avoid storing the conflating position data on collateral custody (in case of shorts/virtual assets) to address the ineffciencies with keeping "track of all trade coin positions in stablecoin custody (i.e. via array), which is inefficient" we have a different solution to model many-many relationship between multiple trade and collateral custodies with an addional state-account as a link allowing us have multiple assets as collateral for each market and trade side. Real trouble starts when you compose over the current program and run into "ix too large" errors. So no, Flash Trade will serve as many users as possible with the best capital efficiency.

[askibin]

yeah, in order to track aggregate pnl properly, you need to:

1. keep track of weighted entry price and amount for both longs and shorts in the trading custody (because a major part of pnl depends on entry-exit price difference)
2. separately keep track of the stablecoin collateral amount to calculate interest rate
3. keep track of the position's locked amount in either the trade custody or collateral custody (depending on long/short or virtual) to ensure max profit is calculated properly So 1. and 2. are easy. While 3. not so. Position stats currently store aggregated positions in trade custodies, but there is no information about which collateral was used (USDC, USDT, etc). Currently, the locked amount represents the trade token amount, which, as you pointed out, is not correct for shorts/virtual. But that was a deliberate decision in favor of simplicity (there is a bug that you uncovered, but it is unrelated to this convo). So the solution would be to do something similar to what Flash did or to track positions with different collateral custody separately. I.e. trade custody will have an array of PositionStats and get_assets_under_management_usd() will loop over and pass proper collateral custody to get_pnl_usd(). The major problem with this is it doesn't scale well to the large number of stablecoin custodies.