search

solana-labs / perpetuals

Solana perpetuals reference implementation
Other
74 stars 43 forks source link

Update pool aum usd #23

Closed adrena-orex closed 1 year ago

adrena-orex commented 1 year ago

Issue

In fee calculation, ratios are computed in order to increase/decrease fees:

https://github.com/solana-labs/perpetuals/blob/43ed8424e816c9a89285f4f7267a52574fd7e651/programs/perpetuals/src/state/pool.rs#L937

https://github.com/solana-labs/perpetuals/blob/43ed8424e816c9a89285f4f7267a52574fd7e651/programs/perpetuals/src/state/pool.rs#L813

If we look at get_current_ratio function, we use the latest custody token price, but an outdated aum_usd.

        let ratio = math::checked_as_u64(math::checked_div(
            math::checked_mul(
                token_price.get_asset_amount_usd(custody.assets.owned, custody.decimals)? as u128,
                Perpetuals::BPS_POWER,
            )?,
            self.aum_usd,
        )?)?;

Soluce

pool.aum_usd should be refreshed at the start of each ix paying fees. So when fees are calculated, the pool.aum_usd is accurate and ratios are corrects.

Demonstration

Imagine the initial state of the pool to be:

Now after some time, prices changes, and users execute add_collateral

New prices:

When the calculation run in add_collateral for its ETH position, it will calculate the current_ratio as:

let ratio = math::checked_as_u64(math::checked_div(
       math::checked_mul(
           token_price.get_asset_amount_usd(custody.assets.owned, custody.decimals)? as u128,
            Perpetuals::BPS_POWER,
        )?,
        self.aum_usd,
 )?)?;

current_ratio = ((1_600 100) 10_000) / 4_150_000 = 385.542168675

when it should have calculated the current ratio using the updated pool.aum_usd such as:

Updated pool.aum_usd: (1_600 100) + (32_000 100) + 1_000_000 = 4_360_000

current_ratio = ((1_600 100) 10_000) / 4_360_000 = 366.972477064

Note

This PR also include a refresh of pool.aum_usd at the end of close_position and liquidate because I think theses both ix may have impactfull impact on pool.aum_usd due to the moving collateral and change of assets.owned

askibin commented 1 year ago

I agree, I didn't include AUM updates in the original implementation because I wasn't sure if the difference in collected fees justifies heavier instructions (in both compute units and number of accounts passed). But we can include that it if you think it is necessary. Also please make sure these commands succeed:

anchor test -- --features test
cargo clippy +nightly --workspace --all-targets -- --deny=warnings
adrena-orex commented 1 year ago

Fixed clippy warnings 🙏

cargo test -- --nocapture runs fine.

adrena-orex commented 1 year ago

Having issues running anchor test -- --features test in local 🤔 (checked with 1.14.18 as well)

orex> anchor test -- --features test
Warning: cargo-build-bpf is deprecated. Please, use cargo-build-sbf
cargo-build-bpf child: /Users/orex/.local/share/solana/install/active_release/bin/cargo-build-sbf --features test --arch bpf
   Compiling perpetuals v0.1.0 (/Users/orex/work/perpetuals-copy-to-do-pr-on-main/programs/perpetuals)
    Finished release [optimized] target(s) in 13.09s

Found a 'test' script in the Anchor.toml. Running it as a test suite!

Running test suite: "/Users/orex/work/perpetuals-copy-to-do-pr-on-main/Anchor.toml"

Unable to get latest blockhash. Test validator does not look started. Check .anchor/test-ledger/test-ledger-log.txt for errors. Consider increasing [test.startup_wait] in Anchor.toml.
Stake By Version:
1.16.4  -    2 current validators (24.99%)
1.14.19 -    5 current validators (74.93%)   1 delinquent validators ( 0.07%)
1.14.18 -    1 current validators ( 0.00%)
1.14.16 -    0 current validators ( 0.00%)   1 delinquent validators ( 0.00%)
1.14.13 -    0 current validators ( 0.00%)   1 delinquent validators ( 0.00%)
1.13.6  -    0 current validators ( 0.00%)   1 delinquent validators ( 0.00%)
unknown -    0 current validators ( 0.00%)  34 delinquent validators ( 0.01%)

orex> solana --version
solana-cli 1.14.19 (src:5704dd6e; feat:1879391783)
askibin commented 1 year ago

did you check .anchor/test-ledger/test-ledger-log.txt for errors? also, I'm still thinking if making more instructions super heavy is a good idea. The only problem we solve is making fees more accurate. If we don't update aum then sometimes users will pay more, sometimes less, in a long run it will cancels out. What if you add a permissionless update_aum instruction, so it will be called periodically by crank or from UI if AUM is not updated for a while.

adrena-orex commented 1 year ago

Thx found a fix about my error

Error I got:

orex> cat .anchor/test-ledger/test-ledger-log.txt
Ledger location: .anchor/test-ledger
Log: .anchor/test-ledger/validator.log
Initializing...
Error: failed to start validator: Failed to create ledger at .anchor/test-ledger: blockstore error

Fixed it using:

brew install gnu-tar
# Put this in ~/.zshrc 
export PATH="/opt/homebrew/opt/gnu-tar/libexec/gnubin:$PATH"

https://solana.stackexchange.com/questions/4499/cant-start-solana-test-validator-on-macos-13-0-1/4761#4761

adrena-orex commented 1 year ago

did you check .anchor/test-ledger/test-ledger-log.txt for errors? also, I'm still thinking if making more instructions super heavy is a good idea. The only problem we solve is making fees more accurate. If we don't update aum then sometimes users will pay more, sometimes less, in a long run it will cancels out. What if you add a permissionless update_aum instruction, so it will be called periodically by crank or from UI if AUM is not updated for a while.

Long run it cancels out yes, but I'm more concern about the unfairness to users. One paying more than another when it shouldn't.

Also, right now pool.aum_usd is updated only when adding/remove liquidity, so I guess not very often in real case, leading to inconsistent fees.

Crank is an interesting idea.

The longer the time between two update_aum call is, the wronger the fee calculation becomes.

Looking at asset price changes on pyth, price doesn't shift by huge amount every second, so I think the fee variation should be acceptable for few seconds.

Note:

1 call every 3s using clockwork equals to 10.512 SOL of transaction fees per year. 1 call every 10s using clockwork equals to 3.1536 SOL of transaction fees per year.

Running some numbers

Pool having:

pool.aum_usd== $4_800_000

ratios:

Let's take a swap_in fee of 100 BPS and a swap_out fee of 100 BPS, configured with FeesMode::Linear. Let's swap 1_500 SOL for USDC.

With pool.aum_usd updated:

With ETH price took +0.1%, BTC price took +0.05%, SOL price took +0.2%, USDC price even and non-updated pool.aum_usd:

With ETH price took +1%, BTC price took +0.5%, SOL price took +2%, USDC price even and non-updated pool.aum_usd:

Adrena-Corto commented 1 year ago

I think your example shows that it can drift quite heavily for small price changes, I would go for updating the pool_aum to reflect the state as close as possible, especially if we can rely on clockwork to do that (in our fork we do rely on clockwork already to automate some tasks).

Wdyt @askibin ?

I also believe that keep the pool_aum up to date is important for composition and extensions on the codebase in need of this metric

askibin commented 1 year ago

I propose you add a permissionless update_aum instruction that you can prepend (when construction a tx in the client) before the actual interaction (trade, swap, etc.).

mzs-dev commented 1 year ago

This is defiently better and reduces attack vectors on mutliple fronts however most of position management instructions can be made efficient with better fee modelling while instrcutions associated to the pool distribution need to necessarily update the pool aum instantaneously.

adrena-orex commented 1 year ago

@askibin updated the PR.