Skip to content
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

V2 #38

Draft
wants to merge 43 commits into
base: mainv2
Choose a base branch
from
Draft

V2 #38

wants to merge 43 commits into from

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Mar 22, 2024

This PR aims at implementing savings app V2.
The base branch is changed to be able to merge changes without touching on v1

Changes

Actually using Abstract

All authz calls have been replaced by calls through the proxy contract as intended by Abstract. This allows to remove hacks and to use the Abstract SDK much more deeply than in the previous version. This felt good and rationalizes the code

Adding multiple strategies

This Savings App V2 now uses a Balance Strategy to direct funds to one investment or the other. This balance strategy consists of multiple yield sources. This balance strategy is saved in the application config. When doing deposits, withdrawals and auto-compounding, this app tries to make the investments match the strategy as much as possible.

Autocompound rewards

Autocompound rewards are now a perentage of the rewards withdrawn during auto-compounding. This allows simpler and more fluid interactions !

Status

  • Withdraw only withdraws an equal share from all investments. This tends to keep the same balance between all yield sources.

  • Autocompound has the target behavior --> Withdraw all rewards and then re-deposit all rewards.

  • Rebalance is a function that allows users to change the hard-coded balance strategy. It withdraws removed strategies and deposits withdrawn funds and additional funds into the app

Zoom on the deposit strategy.

In this proposed V2, each strategy needs to be handled separately. When a deposit is made, the following steps apply :

  1. A share for each investment is computed. This is computed using the target strategy as well as the current deposit share of the source compared to the whole deposit. (Not implemented, for now, we only use the config strategy)
  2. This balance strategy is used to split the deposit into the different available yield sources. Each source is assigned a value and the process to swap assets from the deposited assets into each underlying assets is devised
  3. The strategy is splitted into yield sources.They are all handled separately (swapping and depositing assets) to ensure separation of variables and concerns.

Swap strategy

We want to get most efficiently from the deposited coins into the target coins to enter the yield sources. We have created an algorithm that tries to minimise swaps. This is not optimal but allows to avoid un-necessary swaps:

  1. Take all the deposited coins and try to fit as much as possible in the strategies without swaps (optimized)
  2. Swap the remaining coins to the remaining assets (no optimization)

Remarks on the osmosis CL case

In the Savings app previous version, there was a complicated algorithm to swap assets correctly from the deposited amounts to the expected amounts for the liquidity pool. This algorithm is not used in v2. Instead, for a first deposit, the front-end needs to input the correct share ratio for the investment. The more general value distribution algorithm takes care of choosing the necessary swaps to enter the pool correctly.
TODO : The strategies share need to be computed dynamically from the underlying investments. This would allow pool positions to completely change without the users need of rebalancing everytime they want to do additional deposits

Remaining TODO

  • Withdraw should target the strategy (It is very dumb right now)
  • Maybe add a different endpoint that only tweaks the investments shares instead of overwriting the strategy completely
  • Have a separate structure for valid Balance Strategies (handling valid and non-valid objects is a bit of a mess right now)
  • Adding queries to simulate deposits, withdraw and rebalance

@Kayanski Kayanski marked this pull request as draft March 26, 2024 14:42
@CyberHoward
Copy link
Contributor

Have a separate structure for valid Balance Strategies (handling valid and non-valoid objects is a bit of a mess right now)

You can use Addresslike generic for the struct and create type aliases for the checked and non-checked variant. We do that in a few places in the framework.

}
YieldType::Mars(denom) => mars::user_liquidity(deps, denom.clone(), app),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is where we would need to add a query like deposited_value that returns how much is deposited in the yield source and its breakdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already that function !

deps: Deps,
amount: Option<Uint128>,
app: &App,
) -> AppResult<Vec<CosmosMsg>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have a list of tokens returned there as well, i.e. how many and which tokens the dev can expect to receive after executing this msg.

The dev can

  • Query all the values of the enabled yield strategies
  • Calculate how much to withdraw from each, construct msgs
  • Prepare swap messages if needed
  • Deposit into other strategy with (potentially swapped) funds.
    And all without the need of replies

Would that be possible to implement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I have planned a SimulateRebalance query endpoint. I will also do a SimulateDeposit endpoint for those purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replies were here to avoid simulations and have the exact swapped funds deposited instead of relying on estimates that may change during execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to sync on this, I'm not sure what the idea is behind that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replies were here to avoid simulations and have the exact swapped funds deposited instead of relying on estimates that may change during execution.

Simulations during execution are accurate as long as you don't do any action on the pool before you use a result of that simulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the thing. As we're doing potentially multiple swaps, we're not able to trust those simulations. We can try if you prefer. You don't like replies ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense! But in general our rebalance approach should probably be

  1. Withdraw
  2. Swap (Optional)
  3. Deposit

If we add a simulation we can prevent reply 1 -> 2, independent of wether we need to swap or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replies just makes reading the code a bit messier imo and requires state caching

@Kayanski Kayanski changed the base branch from main to mainv2 March 27, 2024 12:46
@CyberHoward
Copy link
Contributor

Should we change namespace to be seperate from abstract?

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 2, 2024

Should we change namespace to be seperate from abstract?

Why ? Seems like abstract did that app no ?

@CyberHoward
Copy link
Contributor

This balance strategy is used to split the deposit into the different available yield sources. Each source is assigned a value and the process to swap assets from the deposited assets into each underlying assets is devised
The strategy is splitted into yield sources.They are all handled separately (swapping and depositing assets) to ensure separation of variables and concerns.

Can we unify the swaps for these? I.e. we know how much we want to allocate to each source and we know which assets they expect. We can then compute the swaps globally and execute them at once, instead of letting each strategy figure out how much to swap individually and possibly having multiple swaps for the same pair.

@CyberHoward
Copy link
Contributor

algorithmically this is kind of a "set" computation.

You have a set of coins that are provided, you compute the set that is required, you remove the union of those two sets and compute the swaps for the difference in the sets.

@CyberHoward
Copy link
Contributor

Have a separate structure for valid Balance Strategies (handling valid and non-valid objects is a bit of a mess right now)

You can use signal generics for this.

struct Checked; 
struct Unchecked;

struct MyValueBase<T> {
  _phantom: PhantomData<T>
}

type MyValue = MyValueBase<Checked>;
type MyValueUnchecked = MyValueBase<Unchecked>;

impl MyValueUnchecked {
  fn check(self) -> MyValue {
  // ...
  }
}

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 4, 2024

struct Checked; 
struct Unchecked;

This syntax is now applied on all the structures with a check functionality

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 5, 2024

This balance strategy is used to split the deposit into the different available yield sources. Each source is assigned a value and the process to swap assets from the deposited assets into each underlying assets is devised
The strategy is splitted into yield sources.They are all handled separately (swapping and depositing assets) to ensure separation of variables and concerns.

Can we unify the swaps for these? I.e. we know how much we want to allocate to each source and we know which assets they expect. We can then compute the swaps globally and execute them at once, instead of letting each strategy figure out how much to swap individually and possibly having multiple swaps for the same pair.

What would a unification bring ? I only see less gas usage, because fees are a percentage of swap values.
With this change, we would :

  1. Compute the different allocations per strategy
  2. Unify all the swaps that do the same thing
  3. Dispatch the remaining swap results to the corresponding strategies.

Is that right ?

Copy link
Contributor

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice progress! Would like to see some gas optimizations.

Not sure how many strategies are we expecting to be used at the same time, but wonder if we can make rebalancing/autocompounding a little bit smarter by not doing everything at once?

Comment on lines +43 to +44
/// We might not use a vector but use a (usize, Vec<AssetShare>) instead to avoid having to pass a full vector everytime
yield_sources_params: Option<Vec<Option<Vec<AssetShare>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Option<Vec<>> is a good thing to use, as empty vector mean technically same as None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a Vec because if it's specified, one element needs to exists for each element in the underlying strategy. I'm not proud of the way params are passed to this function but I don't have identifiers for the strategy elements !

contracts/carrot-app/src/msg.rs Show resolved Hide resolved
}

#[cw_serde]
pub struct PositionResponse {
pub position: Option<Position>,
pub ty: YieldTypeUnchecked,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Yield type is much more complex object, than just reporting it's "type", maybe something like YieldSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YieldSource is already used somewhere else to encompass the shares of each token inside the Yield Source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But agree Type is a bit misleading here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But agree Type is a bit misleading here

and ty as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is renamed to params is that ok ?

contracts/carrot-app/src/handlers/instantiate.rs Outdated Show resolved Hide resolved
contracts/carrot-app/src/handlers/execute.rs Outdated Show resolved Hide resolved
contracts/carrot-app/src/helpers.rs Outdated Show resolved Hide resolved
contracts/carrot-app/src/state.rs Show resolved Hide resolved
contracts/carrot-app/src/yield_sources/mod.rs Outdated Show resolved Hide resolved

let balances = try_proto_to_cosmwasm_coins(vec![pool.asset0.unwrap(), pool.asset1.unwrap()])?;
let liquidity = pool.position.unwrap().liquidity.replace('.', "");
pub fn query_balance(deps: Deps, app: &App) -> AppResult<AssetsBalanceResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add pagination here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we there yet @CyberHoward, especially the balance query would be misleading with pagination (but could still run out of gas)

})
}

fn query_rewards(deps: Deps, _app: &App) -> AppResult<AvailableRewardsResponse> {
let pool = get_osmosis_position(deps)?;
fn query_rewards(deps: Deps, app: &App) -> AppResult<AvailableRewardsResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add pagination here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants