feat: New Lp Interfaces #4313
feat: New Lp Interfaces #4313
Conversation
AlastairHolmes
requested review from
a team and
martin-chainflip
as code owners
AlastairHolmes
requested review from
jerryafr and
acdibble
and removed request for
a team
AlastairHolmes
force-pushed
the
feat/new-lp-interface-design
branch
from
0a3efde
to
06fb77c
Compare
| } | ||
|
|
||
| { | ||
| let initial_sqrt_price = match asset { | ||
| common::Side::Zero => MAX_SQRT_PRICE - 1, |
There was a problem hiding this comment.
did we want to remove the '-1' ?
There was a problem hiding this comment.
yes (should have been done before in previous pr, saw I hadn't before)
There was a problem hiding this comment.
I changed the valid sqrt_prices to include the MAX_SQRT_PRICE in all cases, previously, so some of internal functions were safer to use. As a result MAX_SQRT_PRICE is a valid starting price. Where it wasn't before.
| let initial_sqrt_price = match asset { | ||
| Side::Zero => MAX_SQRT_PRICE - 1, | ||
| let initial_sqrt_price = match order.to_sold_side() { | ||
| Side::Zero => MAX_SQRT_PRICE, |
| ) | ||
| ); | ||
| } | ||
|
|
||
| { | ||
| let initial_sqrt_price = match asset { | ||
| Side::Zero => MAX_SQRT_PRICE - 1, | ||
| let initial_sqrt_price = match order.to_sold_side() { |
| pool_state: old_pool.pool_state.into(), | ||
| }) | ||
| }); | ||
| for (key, pool) in old::Pools::<T>::iter().filter_map(|(old_key, old_pool)| { |
There was a problem hiding this comment.
I think this should use drain otherwise it won't remove the old pools (they will remain stored under the old key).
| @@ -1894,25 +1863,37 @@ impl<T: Config> Pallet<T> { | |||
| pool: &mut Pool<T>, | |||
| asset_pair: &AssetPair, | |||
| lp: &T::AccountId, | |||
| side: Side, | |||
| order: OrderId, | |||
| order: Order, | |||
There was a problem hiding this comment.
nit: I think Side was a better name. Order suggests it's the actual order. (but I can see Side already has another meaning).
How about OrderSide or BuyOrSell or something less ambiguous?
There was a problem hiding this comment.
Order types are generally used to refer to things like limit order / market order / fill or kill etc. Ie. it's a collection of attributes or criteria.
There was a problem hiding this comment.
We could just name the argument side in the LpApi? I don't think the type name is the main issue.
| Deserialize, | ||
| Serialize, | ||
| )] | ||
| pub enum Assets { |
There was a problem hiding this comment.
This is also kind of a confusing name...
There was a problem hiding this comment.
We have have OrderSide (buy/sell), PoolSide (base/quote), AmmSide (one,zero) to disambiguate all of this a bit more clearly?
There was a problem hiding this comment.
This one isn't exposed externally, so as I said I do intend to a cleanup. Unless you can give a better name, I'll leave it for now.
There was a problem hiding this comment.
I did give some better names 😅
But ok to leave it if it's not exposed. What about order? This is one is in the Api.
There was a problem hiding this comment.
I know we want to get this out but once we settle on external-facing terminology it will be harder to change it.
There was a problem hiding this comment.
Not sure what you're suggesting here...
There was a problem hiding this comment.
GitHUb decided to not show your comments
No description provided.