Result<Option<>> rather than Option<Option<>> (#9119)

* Clearer API to code against.

substrate/primitives/runtime/src/offchain/storage.rs

@@ -28,6 +28,25 @@ pub struct StorageValueRef<'a> {
 	kind: StorageKind,
 }
 
+/// Reason for not being able to provide the stored value
+#[derive(Debug, PartialEq, Eq)]
+pub enum StorageRetrievalError {
+	/// Value found but undecodable
+	Undecodable,
+}
+
+/// Possible errors when mutating a storage value.
+#[derive(Debug, PartialEq, Eq)]
+pub enum MutateStorageError<T, E> {
+	/// The underlying db failed to update due to a concurrent modification.
+	/// Contains the new value that was not stored.
+	ConcurrentModification(T),
+	/// The function given to us to create the value to be stored failed.
+	/// May be used to signal that having looked at the existing value,
+	/// they don't want to mutate it.
+	ValueFunctionFailed(E)
+}
+
 impl<'a> StorageValueRef<'a> {
 	/// Create a new reference to a value in the persistent local storage.
 	pub fn persistent(key: &'a [u8]) -> Self {
@@ -58,30 +77,40 @@ impl<'a> StorageValueRef<'a> {
 	/// Retrieve & decode the value from storage.
 	///
 	/// Note that if you want to do some checks based on the value
-	/// and write changes after that you should rather be using `mutate`.
+	/// and write changes after that, you should rather be using `mutate`.
 	///
-	/// The function returns `None` if the value was not found in storage,
-	/// otherwise a decoding of the value to requested type.
-	pub fn get<T: codec::Decode>(&self) -> Option<Option<T>> {
+	/// Returns the value if stored.
+	/// Returns an error if the value could not be decoded.
+	pub fn get<T: codec::Decode>(&self) -> Result<Option<T>, StorageRetrievalError> {
 		sp_io::offchain::local_storage_get(self.kind, self.key)
-			.map(|val| T::decode(&mut &*val).ok())
+			.map(|val| T::decode(&mut &*val)
+			.map_err(|_| StorageRetrievalError::Undecodable))
+			.transpose()
 	}
 
-	/// Retrieve & decode the value and set it to a new one atomically.
+	/// Retrieve & decode the current value and set it to a new value atomically.
+	///
+	/// Function `mutate_val` takes as input the current value and should
+	/// return a new value that is attempted to be written to storage.
 	///
-	/// Function `f` should return a new value that we should attempt to write to storage.
 	/// This function returns:
-	/// 1. `Ok(Ok(T))` in case the value has been successfully set.
-	/// 2. `Ok(Err(T))` in case the value was calculated by the passed closure `f`,
-	///    but it could not be stored.
-	/// 3. `Err(_)` in case `f` returns an error.
-	pub fn mutate<T, E, F>(&self, f: F) -> Result<Result<T, T>, E> where
+	/// 1. `Ok(T)` in case the value has been successfully set.
+	/// 2. `Err(MutateStorageError::ConcurrentModification(T))` in case the value was calculated
+	/// by the passed closure `mutate_val`, but it could not be stored.
+	/// 3. `Err(MutateStorageError::ValueFunctionFailed(_))` in case `mutate_val` returns an error.
+	pub fn mutate<T, E, F>(&self, mutate_val: F) -> Result<T, MutateStorageError<T,E>> where
 		T: codec::Codec,
-		F: FnOnce(Option<Option<T>>) -> Result<T, E>
+		F: FnOnce(Result<Option<T>, StorageRetrievalError>) -> Result<T, E>
 	{
 		let value = sp_io::offchain::local_storage_get(self.kind, self.key);
-		let decoded = value.as_deref().map(|mut v| T::decode(&mut v).ok());
-		let val = f(decoded)?;
+		let decoded = value.as_deref()
+			.map(|mut bytes| {
+				T::decode(&mut bytes)
+					.map_err(|_| StorageRetrievalError::Undecodable)
+			}).transpose();
+
+		let val = mutate_val(decoded).map_err(|err| MutateStorageError::ValueFunctionFailed(err))?;
+
 		let set = val.using_encoded(|new_val| {
 			sp_io::offchain::local_storage_compare_and_set(
 				self.kind,
@@ -90,11 +119,10 @@ impl<'a> StorageValueRef<'a> {
 				new_val,
 			)
 		});
-
 		if set {
-			Ok(Ok(val))
+			Ok(val)
 		} else {
-			Ok(Err(val))
+			Err(MutateStorageError::ConcurrentModification(val))
 		}
 	}
 }
@@ -117,12 +145,12 @@ mod tests {
 		t.execute_with(|| {
 			let val = StorageValue::persistent(b"testval");
 
-			assert_eq!(val.get::<u32>(), None);
+			assert_eq!(val.get::<u32>(), Ok(None));
 
 			val.set(&15_u32);
 
-			assert_eq!(val.get::<u32>(), Some(Some(15_u32)));
-			assert_eq!(val.get::<Vec<u8>>(), Some(None));
+			assert_eq!(val.get::<u32>(), Ok(Some(15_u32)));
+			assert_eq!(val.get::<Vec<u8>>(), Err(StorageRetrievalError::Undecodable));
 			assert_eq!(
 				state.read().persistent_storage.get(b"testval"),
 				Some(vec![15_u8, 0, 0, 0])
@@ -140,12 +168,12 @@ mod tests {
 			let val = StorageValue::persistent(b"testval");
 
 			let result = val.mutate::<u32, (), _>(|val| {
-				assert_eq!(val, None);
+				assert_eq!(val, Ok(None));
 
 				Ok(16_u32)
 			});
-			assert_eq!(result, Ok(Ok(16_u32)));
-			assert_eq!(val.get::<u32>(), Some(Some(16_u32)));
+			assert_eq!(result, Ok(16_u32));
+			assert_eq!(val.get::<u32>(), Ok(Some(16_u32)));
 			assert_eq!(
 				state.read().persistent_storage.get(b"testval"),
 				Some(vec![16_u8, 0, 0, 0])
@@ -153,10 +181,10 @@ mod tests {
 
 			// mutate again, but this time early-exit.
 			let res = val.mutate::<u32, (), _>(|val| {
-				assert_eq!(val, Some(Some(16_u32)));
+				assert_eq!(val, Ok(Some(16_u32)));
 				Err(())
 			});
-			assert_eq!(res, Err(()));
+			assert_eq!(res, Err(MutateStorageError::ValueFunctionFailed(())));
 		})
 	}
 }

substrate/primitives/runtime/src/offchain/storage_lock.rs

@@ -61,7 +61,7 @@
 //! }
 //! ```
 
-use crate::offchain::storage::StorageValueRef;
+use crate::offchain::storage::{StorageRetrievalError, MutateStorageError, StorageValueRef};
 use crate::traits::AtLeast32BitUnsigned;
 use codec::{Codec, Decode, Encode};
 use sp_core::offchain::{Duration, Timestamp};
@@ -279,19 +279,20 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 
 	/// Extend active lock's deadline
 	fn extend_active_lock(&mut self) -> Result<<L as Lockable>::Deadline, ()> {
-		let res = self.value_ref.mutate(|s: Option<Option<L::Deadline>>| -> Result<<L as Lockable>::Deadline, ()> {
+		let res = self.value_ref.mutate(
+			|s: Result<Option<L::Deadline>, StorageRetrievalError>| -> Result<<L as Lockable>::Deadline, ()> {
 			match s {
 				// lock is present and is still active, extend the lock.
-				Some(Some(deadline)) if !<L as Lockable>::has_expired(&deadline) =>
+				Ok(Some(deadline)) if !<L as Lockable>::has_expired(&deadline) =>
 					Ok(self.lockable.deadline()),
 				// other cases
 				_ => Err(()),
 			}
 		});
 		match res {
-			Ok(Ok(deadline)) => Ok(deadline),
-			Ok(Err(_)) => Err(()),
-			Err(e) => Err(e),
+			Ok(deadline) => Ok(deadline),
+			Err(MutateStorageError::ConcurrentModification(_)) => Err(()),
+			Err(MutateStorageError::ValueFunctionFailed(e)) => Err(e),
 		}
 	}
 
@@ -301,25 +302,25 @@ impl<'a, L: Lockable> StorageLock<'a, L> {
 		new_deadline: L::Deadline,
 	) -> Result<(), <L as Lockable>::Deadline> {
 		let res = self.value_ref.mutate(
-			|s: Option<Option<L::Deadline>>|
+			|s: Result<Option<L::Deadline>, StorageRetrievalError>|
 			-> Result<<L as Lockable>::Deadline, <L as Lockable>::Deadline> {
 				match s {
 					// no lock set, we can safely acquire it
-					None => Ok(new_deadline),
+					Ok(None) => Ok(new_deadline),
 					// write was good, but read failed
-					Some(None) => Ok(new_deadline),
+					Err(_) => Ok(new_deadline),
 					// lock is set, but it is expired. We can re-acquire it.
-					Some(Some(deadline)) if <L as Lockable>::has_expired(&deadline) =>
+					Ok(Some(deadline)) if <L as Lockable>::has_expired(&deadline) =>
 						Ok(new_deadline),
 					// lock is present and is still active
-					Some(Some(deadline)) => Err(deadline),
+					Ok(Some(deadline)) => Err(deadline),
 				}
 			},
 		);
 		match res {
-			Ok(Ok(_)) => Ok(()),
-			Ok(Err(deadline)) => Err(deadline),
-			Err(e) => Err(e),
+			Ok(_) => Ok(()),
+			Err(MutateStorageError::ConcurrentModification(deadline)) => Err(deadline),
+			Err(MutateStorageError::ValueFunctionFailed(e)) => Err(e),
 		}
 	}
 
@@ -488,14 +489,14 @@ mod tests {
 
 				val.set(&VAL_1);
 
-				assert_eq!(val.get::<u32>(), Some(Some(VAL_1)));
+				assert_eq!(val.get::<u32>(), Ok(Some(VAL_1)));
 			}
 
 			{
 				let _guard = lock.lock();
 				val.set(&VAL_2);
 
-				assert_eq!(val.get::<u32>(), Some(Some(VAL_2)));
+				assert_eq!(val.get::<u32>(), Ok(Some(VAL_2)));
 			}
 		});
 		// lock must have been cleared at this point
@@ -518,7 +519,7 @@ mod tests {
 
 			val.set(&VAL_1);
 
-			assert_eq!(val.get::<u32>(), Some(Some(VAL_1)));
+			assert_eq!(val.get::<u32>(), Ok(Some(VAL_1)));
 
 			guard.forget();
 		});