feat(runtime): use committee origin for pallet asset-index by clearloop · Pull Request #300 · ChainSafe/PINT

clearloop · 2021-08-30T09:23:41Z

Changes

Tests

CI

Issues

clearloop · 2021-08-31T06:54:44Z

This PR simply add an interface for creating feed in pallet price-feed

The benchmarks of asset-index is still broken with:

Error:                                                                                                                                 
* Pallet: AssetIndex                                                                                                                   
* Benchmark: deposit                                                                                                                   
* Lowest_range_values: []                                                                                                              
* Highest_range_values: []                                                                                                             
* Steps: []                                                                                                                            
* Repeat: 1                                                                                                                            
* Verify: true                                                                                                                         
* Error message: An overflow would occur

Which is from the pallet-chainlink-feed, tried submit round in the chainlink-feed node-template directly, not working as well with Error::ReportingOrder

mattsse

I don't really understand the price feed benchmark extensions and I don't think we should add this like that because the PriceFeedBenchmarks is very invasive. But maybe I didn't get the problem.

mattsse · 2021-08-31T07:49:41Z

pallets/price-feed/src/lib.rs

+	#[cfg(feature = "runtime-benchmarks")]
+	impl<T: Config> PriceFeedBenchmarks<T::AccountId, T::AssetId> for Pallet<T> {
+		fn create_feed(
+			caller: <T as frame_system::Config>::AccountId,
+			asset_id: T::AssetId,
+		) -> DispatchResultWithPostInfo {
+			use frame_benchmarking::vec;
+
+			pallet_chainlink_feed::Pallet::<T>::set_feed_creator(
+				<frame_system::Origin<T>>::Signed(pallet_chainlink_feed::Pallet::<T>::pallet_admin()).into(),
+				caller.clone(),
+			)?;
+
+			pallet_chainlink_feed::Pallet::<T>::create_feed(
+				<frame_system::Origin<T>>::Signed(caller.clone()).into(),
+				100u32.into(),
+				Zero::zero(),
+				(1u8.into(), 100u8.into()),
+				1u8.into(),
+				8u8,
+				vec![1; T::StringLimit::get() as usize],
+				Zero::zero(),
+				vec![(caller.clone(), caller.clone())],
+				None,
+				None,
+			)?;
+
+			let feed_id = <pallet_chainlink_feed::FeedCounter<T>>::get() - 1.into();
+			AssetFeeds::<T>::insert(&asset_id, feed_id);
+			pallet_chainlink_feed::Pallet::<T>::submit(
+				<frame_system::Origin<T>>::Signed(caller.clone()).into(),
+				feed_id,
+				1_u32.into(),
+				42.into(),
+			)?;
+			Ok(().into())
+		}
+	}
+


This is essentially a mock?
I'm rather against adding this. Can you extend on why this is necessary, we should fix this upstream if this is a problem of the chainlink pallet.

Yep, I'm thinking of fixing it in the chainlink repo and open another issue for this

mattsse · 2021-08-31T07:58:46Z

Also there is already the Feedbuilder we use for price feed tests https://github.com/ChainSafe/PINT/blob/bb27364ebe9cc7021860630aa7502e80bc6b7999/pallets/price-feed/src/mock.rs
can't we use that instead?

clearloop · 2021-08-31T08:57:40Z

Also there is already the Feedbuilder we use for price feed tests https://github.com/ChainSafe/PINT/blob/bb27364ebe9cc7021860630aa7502e80bc6b7999/pallets/price-feed/src/mock.rs
can't we use that instead?

The dependencies benchmarks depend on are not as same as the tests, they are using the real runtimes, and whatever we have MockPriceFeed or Feedbuilder, the benchmarks don't care about these, they only work for tests ( including the tests in benchmarks ), but not work for benchmarks,

So if we want to generate weights for extrinsics like deposit in pallet asset-index, we need to create_feed and submit a round in the pallet asset-index, it used to be workable because we had a cache for the price of a new asset before

mattsse · 2021-08-31T09:27:33Z

gotcha,
this is probably solvable because the PriceFeed pallet is also chainlink_feed_pallet::Pallet so you should be able to access that, and we need to enable this in the runtime section as well?
this could work?

clearloop · 2021-08-31T09:32:11Z

gotcha,
this is probably solvable because the PriceFeed pallet is also chainlink_feed_pallet::Pallet so you should be able to access that, and we need to enable this in the runtime section as well?
this could work?

Yep, and this PriceFeedBenchmarks is for this creating feed requirement, tried to re-generate weights for all pallets, both asset-index and saft-registry have the overflow problem mentioned above, not certain about why it happens currently now

Considering if it is caused by the chainlink pallet is that I had tried to submit a feed directly on the UI with the node-template of chainlink-feed in master branch, and it has problems, but after seeing the saft-registry has the same overflow problem, I'm thinking of if there are any other problems

mattsse · 2021-08-31T09:46:05Z

Okay seems we need to track this down.
Can you please split the committee origin and create a separate PR for this?

clearloop · 2021-08-31T09:49:56Z

Okay seems we need to track this down.
Can you please split the committee origin and create a separate PR for this?

Yep, in #305 now =.=

mattsse

lgtm

clearloop added 3 commits August 30, 2021 17:22

chore(asset-index): clean outdated todos

4428b50

chore(runtime): move EnsureApprovedByCommittee to crate common

285dbcc

feat(runtime): use committee origin as admin origin in asset-index

e34d062

clearloop changed the title ~~feat(runtime): check and update origins for all pallets~~ feat(runtime): use committee origin for pallet asset-index Aug 30, 2021

feat(price-feed): supports creating feed in feature runtime-benchmarks

05eca88

dutterbutter added the WIP Work in progress - do not review or merge label Aug 30, 2021

clearloop added 3 commits August 30, 2021 23:46

feat(price-feed): add trait PriceFeedBenchmark

007cc05

feat(price-feed): fix tests

6b55b0b

feat(price-feed): clean dependencies

ad41eb1

clearloop marked this pull request as ready for review August 31, 2021 06:55

clearloop requested a review from mattsse August 31, 2021 06:55

clearloop added needs review PR needs reviewing and removed WIP Work in progress - do not review or merge labels Aug 31, 2021

mattsse suggested changes Aug 31, 2021

View reviewed changes

mattsse added needs changes PR needs changes and removed needs review PR needs reviewing labels Aug 31, 2021

Merge branch 'main' into cl/origins

7928434

mattsse approved these changes Aug 31, 2021

View reviewed changes

mattsse merged commit 8f0a83e into main Aug 31, 2021

mattsse deleted the cl/origins branch August 31, 2021 12:17

clearloop mentioned this pull request Sep 2, 2021

Fix the benchmarks of pallet saft-registry #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(runtime): use committee origin for pallet asset-index#300

feat(runtime): use committee origin for pallet asset-index#300
mattsse merged 8 commits intomainfrom
cl/origins

clearloop commented Aug 30, 2021 •

edited

Loading

Uh oh!

clearloop commented Aug 31, 2021 •

edited

Loading

Uh oh!

mattsse left a comment

Uh oh!

mattsse Aug 31, 2021

Uh oh!

clearloop Aug 31, 2021

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021 •

edited

Loading

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021

Uh oh!

mattsse left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

clearloop commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Issues

Uh oh!

clearloop commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

clearloop Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse commented Aug 31, 2021

Uh oh!

clearloop commented Aug 31, 2021

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clearloop commented Aug 30, 2021 •

edited

Loading

clearloop commented Aug 31, 2021 •

edited

Loading

clearloop commented Aug 31, 2021 •

edited

Loading