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

Liquidity pools: Add UTs to for update_token_price() #1890

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lemunozm
Copy link
Contributor

Description

UTs for update_token_price()

NOTE: It's not directly related to LPv2

Changes and Descriptions

  • Token price simplification removed some unused types & traits.
  • Remove the IT related to this.
  • Add UTs covering the possible errors.

@lemunozm lemunozm added I4-tests Test needs fixing or improving. crcl-protocol Circle protocol related. I11-cleaning No mandatory issue that leave the repo more readable/organized labels Jun 27, 2024
@lemunozm lemunozm self-assigned this Jun 27, 2024
Comment on lines -209 to -233
/// A trait that can be used to retrieve the current price for a currency
pub struct CurrencyPair<CurrencyId> {
pub base: CurrencyId,
pub quote: CurrencyId,
}

pub struct PriceValue<CurrencyId, Rate, Moment> {
pub pair: CurrencyPair<CurrencyId>,
pub price: Rate,
pub last_updated: Moment,
}

pub trait CurrencyPrice<CurrencyId> {
type Rate;
type Moment;

/// Retrieve the latest price of `base` currency, denominated in the `quote`
/// currency If `quote` currency is not passed, then the default `quote`
/// currency is used (when possible)
fn get_latest(
base: CurrencyId,
quote: Option<CurrencyId>,
) -> Option<PriceValue<CurrencyId, Self::Rate, Self::Moment>>;
}

Copy link
Contributor Author

@lemunozm lemunozm Jun 27, 2024

Choose a reason for hiding this comment

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

This is not used, but if it's intended to be used very soon, I can revert this. Otherwise, I would left removed.

wischli
wischli previously approved these changes Jun 27, 2024
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 47.49%. Comparing base (2f08ae7) to head (0868b0a).

Files Patch % Lines
pallets/liquidity-pools/src/lib.rs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1890      +/-   ##
==========================================
+ Coverage   47.38%   47.49%   +0.11%     
==========================================
  Files         176      176              
  Lines       13305    13304       -1     
==========================================
+ Hits         6304     6319      +15     
+ Misses       7001     6985      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lemunozm lemunozm enabled auto-merge (squash) June 27, 2024 22:10
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Please do not merge until hieronx answered. I also would like to close the conversion in the same run too

let price = prices.get(tranche_index).cloned()?;

Some(PriceValue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hieronx you added this change. Why did you wanted to have the TrancheCurrency and the PoolCurrency named here?

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 fully remember. I think we just set up the PriceValue for some other purpose, and then used that here.

@@ -426,7 +426,7 @@ pub mod pallet {
// TODO(future): Once we diverge from 1-to-1 conversions for foreign and pool
// currencies, this price must be first converted into the currency_id and then
// re-denominated to 18 decimals (i.e. `Ratio` precision)
let price_value = T::TrancheTokenPrice::get(pool_id, tranche_id)
let (price, computed_at) = T::TrancheTokenPrice::get_price(pool_id, tranche_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lemunozm can we in the same run also close this todo and computed the conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's super straightforward. Do we need to add the order-book dependency to know the market price used for foreign/pool currencies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it isn't. But we are not only touching the UTs here but also chaning the return types, naming etc. Then we could also close the todo IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: 15424a8 @mustermeiszer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. I4-tests Test needs fixing or improving. I11-cleaning No mandatory issue that leave the repo more readable/organized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants