Changed the way values are updated (#720)

* Changed the way values are updated. Instead of updating on "get", values are updated on feed.

* minor improvement

* Addressed some PR comments

* Simplified get_all_values function.

* formatted

Co-authored-by: Roy Yang <[email protected]>

Author: syan095

oracle/src/lib.rs

@@ -119,19 +119,13 @@ pub mod module {
 	pub type RawValues<T: Config<I>, I: 'static = ()> =
 		StorageDoubleMap<_, Twox64Concat, T::AccountId, Twox64Concat, T::OracleKey, TimestampedValueOf<T, I>>;
 
-	/// True if Self::values(key) is up to date, otherwise the value is stale
-	#[pallet::storage]
-	#[pallet::getter(fn is_updated)]
-	pub type IsUpdated<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Twox64Concat, <T as Config<I>>::OracleKey, bool, ValueQuery>;
-
-	/// Combined value, may not be up to date
+	/// Up to date combined value from Raw Values
 	#[pallet::storage]
 	#[pallet::getter(fn values)]
 	pub type Values<T: Config<I>, I: 'static = ()> =
 		StorageMap<_, Twox64Concat, <T as Config<I>>::OracleKey, TimestampedValueOf<T, I>>;
 
-	/// If an oracle operator has feed a value in this block
+	/// If an oracle operator has fed a value in this block
 	#[pallet::storage]
 	pub(crate) type HasDispatched<T: Config<I>, I: 'static = ()> =
 		StorageValue<_, OrderedSet<T::AccountId, T::MaxHasDispatchedSize>, ValueQuery>;
@@ -182,42 +176,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			.collect()
 	}
 
-	/// Returns fresh combined value if has update, or latest combined
-	/// value.
-	///
-	/// Note this will update values storage if has update.
+	/// Fetch current combined value.
 	pub fn get(key: &T::OracleKey) -> Option<TimestampedValueOf<T, I>> {
-		if Self::is_updated(key) {
-			<Values<T, I>>::get(key)
-		} else {
-			let timestamped = Self::combined(key)?;
-			<Values<T, I>>::insert(key, timestamped.clone());
-			IsUpdated::<T, I>::insert(key, true);
-			Some(timestamped)
-		}
-	}
-
-	/// Returns fresh combined value if has update, or latest combined
-	/// value.
-	///
-	/// This is a no-op function which would not change storage.
-	pub fn get_no_op(key: &T::OracleKey) -> Option<TimestampedValueOf<T, I>> {
-		if Self::is_updated(key) {
-			Self::values(key)
-		} else {
-			Self::combined(key)
-		}
+		Self::values(key)
 	}
 
 	#[allow(clippy::complexity)]
 	pub fn get_all_values() -> Vec<(T::OracleKey, Option<TimestampedValueOf<T, I>>)> {
-		<Values<T, I>>::iter()
-			.map(|(key, _)| key)
-			.map(|key| {
-				let v = Self::get_no_op(&key);
-				(key, v)
-			})
-			.collect()
+		<Values<T, I>>::iter().map(|(k, v)| (k, Some(v))).collect()
 	}
 
 	fn combined(key: &T::OracleKey) -> Option<TimestampedValueOf<T, I>> {
@@ -247,7 +213,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				timestamp: now,
 			};
 			RawValues::<T, I>::insert(&who, &key, timestamped);
-			IsUpdated::<T, I>::remove(&key);
+
+			// Update `Values` storage if `combined` yielded result.
+			if let Some(combined) = Self::combined(key) {
+				<Values<T, I>>::insert(key, combined);
+			}
 
 			T::OnNewData::on_new_data(&who, key, value);
 		}
@@ -262,9 +232,6 @@ impl<T: Config<I>, I: 'static> ChangeMembers<T::AccountId> for Pallet<T, I> {
 		for removed in outgoing {
 			RawValues::<T, I>::remove_prefix(removed, None);
 		}
-
-		// not bothering to track which key needs recompute, just update all
-		IsUpdated::<T, I>::remove_all(None);
 	}
 
 	fn set_prime(_prime: Option<T::AccountId>) {
@@ -279,7 +246,7 @@ impl<T: Config<I>, I: 'static> DataProvider<T::OracleKey, T::OracleValue> for Pa
 }
 impl<T: Config<I>, I: 'static> DataProviderExtended<T::OracleKey, TimestampedValueOf<T, I>> for Pallet<T, I> {
 	fn get_no_op(key: &T::OracleKey) -> Option<TimestampedValueOf<T, I>> {
-		Self::get_no_op(key)
+		Self::get(key)
 	}
 
 	#[allow(clippy::complexity)]

oracle/src/tests.rs

@@ -100,29 +100,6 @@ fn should_not_feed_values_from_root_directly() {
 	});
 }
 
-#[test]
-fn should_update_is_updated() {
-	new_test_ext().execute_with(|| {
-		let key: u32 = 50;
-		assert!(!ModuleOracle::is_updated(key));
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(1), vec![(key, 1000)]));
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(2), vec![(key, 1000)]));
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(3), vec![(key, 1000)]));
-		assert!(!ModuleOracle::is_updated(key));
-		assert_eq!(
-			ModuleOracle::get(&key).unwrap(),
-			TimestampedValue {
-				value: 1000,
-				timestamp: 12345
-			}
-		);
-		assert!(ModuleOracle::is_updated(key));
-		ModuleOracle::on_finalize(1);
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(1), vec![(key, 1000)]));
-		assert!(!ModuleOracle::is_updated(key));
-	});
-}
-
 #[test]
 fn should_read_raw_values() {
 	new_test_ext().execute_with(|| {
@@ -261,35 +238,34 @@ fn change_member_should_work() {
 }
 
 #[test]
-fn should_clear_is_updated_on_change_member() {
+fn should_clear_data_for_removed_members() {
 	new_test_ext().execute_with(|| {
 		assert_ok!(ModuleOracle::feed_values(Origin::signed(1), vec![(50, 1000)]));
 		assert_ok!(ModuleOracle::feed_values(Origin::signed(2), vec![(50, 1000)]));
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(3), vec![(50, 1000)]));
-
-		assert_eq!(
-			ModuleOracle::get(&50).unwrap(),
-			TimestampedValue {
-				value: 1000,
-				timestamp: 12345
-			}
-		);
-		assert!(ModuleOracle::is_updated(50));
 
 		ModuleOracle::change_members_sorted(&[4], &[1], &[2, 3, 4]);
 
-		assert!(!ModuleOracle::is_updated(50));
+		assert_eq!(ModuleOracle::raw_values(&1, 50), None);
 	});
 }
 
 #[test]
-fn should_clear_data_for_removed_members() {
+fn values_are_updated_on_feed() {
 	new_test_ext().execute_with(|| {
-		assert_ok!(ModuleOracle::feed_values(Origin::signed(1), vec![(50, 1000)]));
+		assert_ok!(ModuleOracle::feed_values(Origin::signed(1), vec![(50, 900)]));
 		assert_ok!(ModuleOracle::feed_values(Origin::signed(2), vec![(50, 1000)]));
 
-		ModuleOracle::change_members_sorted(&[4], &[1], &[2, 3, 4]);
+		assert_eq!(ModuleOracle::values(50), None);
 
-		assert_eq!(ModuleOracle::raw_values(&1, 50), None);
+		// Upon the third price feed, the value is updated immediately after `combine`
+		// can produce valid result.
+		assert_ok!(ModuleOracle::feed_values(Origin::signed(3), vec![(50, 1100)]));
+		assert_eq!(
+			ModuleOracle::values(50),
+			Some(TimestampedValue {
+				value: 1000,
+				timestamp: 12345,
+			})
+		);
 	});
 }