Skip to content

feat(asset-index): store initial price pair#106

Merged
clearloop merged 7 commits intomainfrom
cl/initial-price-pair
Jun 15, 2021
Merged

feat(asset-index): store initial price pair#106
clearloop merged 7 commits intomainfrom
cl/initial-price-pair

Conversation

@clearloop
Copy link
Contributor

@clearloop clearloop commented Jun 10, 2021

Changes

  • Add checks for dup assets
  • store initial price pair
  • tests
  • benchmarks

Tests

CI

Issues

Ref #105
Closes #93
Closes #94
Closes #95

@clearloop clearloop added the WIP Work in progress - do not review or merge label Jun 10, 2021
@clearloop clearloop marked this pull request as ready for review June 11, 2021 08:17
@clearloop clearloop added needs review PR needs reviewing and removed WIP Work in progress - do not review or merge labels Jun 11, 2021
///
/// * insert: adding a new asset with no price pair been set yet
/// * remove: an asset has its price pair, remove automatically
pub type InitialPricePair<T: Config> =
Copy link
Contributor Author

@clearloop clearloop Jun 11, 2021

Choose a reason for hiding this comment

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

Use InitialPricePairs but not submitting new feeds

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

LGTM, minor nits

/// This storage stores the initial price pair for quote assets based on `SelfAssetId`
///
/// * insert: adding a new asset with no price pair been set yet
/// * remove: an asset has its price pair, remove automatically
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this around, since this is only a single entry?
removing it is probably not worth it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with keeping this without removing

I don't have a certain idea about this actually, this confused me a lot since submitting a new feed automatically while adding a new asset is not suitable

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, chainlink feeds are kept independently from the assets, and the price pair pallet merely keeps a mapping.
you can also have a look at acala's chainlink integration https://github.com/AcalaNetwork/Acala/pull/1073/files

let caller = ensure_signed(origin)?;

// Store intial price pair if not exists
T::PriceFeed::ensure_price(asset_id, Price::from_inner((value / units).into()))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

would safe math make sense 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.

Had noticed but forgot

@mattsse mattsse added approved PR approved to merge and removed needs review PR needs reviewing labels Jun 15, 2021
@clearloop clearloop merged commit 49ce268 into main Jun 15, 2021
@clearloop clearloop deleted the cl/initial-price-pair branch June 15, 2021 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR approved to merge

Projects

None yet

2 participants