review pallet-chainlink

pallet-chainlink/Cargo.toml

@@ -6,6 +6,7 @@ edition = "2018"
 
 [dependencies]
 codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] }
+# REVIEW: Use version released on crates.io.
 sp-std = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' }
 sp-runtime = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' }
 frame-support = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' }

pallet-chainlink/src/lib.rs

@@ -20,7 +20,7 @@ use sp_std::if_std;
 #[warn(unused_imports)]
 use codec::{Codec, Decode, Encode};
 use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, Parameter, dispatch::DispatchResult};
-use frame_support::traits::{Get, ReservableCurrency};
+use frame_support::traits::{Get, ReservableCurrency, Currency};
 use sp_std::prelude::*;
 use sp_runtime::traits::Dispatchable;
 use frame_system::{ensure_signed, RawOrigin};
@@ -42,6 +42,9 @@ pub trait Trait: frame_system::Trait {
 	type ValidityPeriod: Get<Self::BlockNumber>;
 }
 
+// REVIEW: Use this for transfering currency.
+type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
+
 // Uniquely identify a request's specification understood by an Operator
 pub type SpecIndex = u32;
 // Uniquely identify a request for a considered Operator
@@ -60,17 +63,19 @@ decl_storage! {
 
 		// A map of details of each running request
 		// TODO migrate to 'natural' hasher once migrated to 2.0
-		pub Requests get(fn request): linked_map hasher(blake2_256) RequestIdentifier => (T::AccountId, Vec<T::Callback>, T::BlockNumber, u32);
+		// REVIEW: Consider using a struct for the Requests instead of a tuple to increase
+		//         readability.
+		pub Requests get(fn request): linked_map hasher(blake2_256) RequestIdentifier => (T::AccountId, Vec<T::Callback>, T::BlockNumber, BalanceOf<T>);
     }
 }
 
 decl_event!(
-	pub enum Event<T> where AccountId = <T as frame_system::Trait>::AccountId {
+	pub enum Event<T> where AccountId = <T as frame_system::Trait>::AccountId, Balance = BalanceOf<T> {
 		// A request has been accepted. Corresponding fee paiement is reserved
-		OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec<u8>, Vec<u8>, u32),
+		OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec<u8>, Vec<u8>, Balance),
 
 		// A request has been answered. Corresponding fee paiement is transfered
-		OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec<u8>, u32),
+		OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec<u8>, Balance),
 
 		// A new operator has been registered
 		OperatorRegistered(AccountId),
@@ -106,6 +111,7 @@ decl_module! {
 
 		fn deposit_event() = default;
 
+		// REVIEW: Use `///` instead of `//` to make these doc comments that are part of the crate documentation.
 		// Register a new Operator.
 		// Fails with `OperatorAlreadyRegistered` if this Operator (identified by `origin`) has already been registered.
 		pub fn register_operator(origin) -> DispatchResult {
@@ -138,18 +144,30 @@ decl_module! {
 		// If provided fee is sufficient, Operator must send back the request result in `callback` Extrinsic which then will dispatch back to the request originator callback identified by `callback`.
 		// The fee is `reserved` and only actually transferred when the result is provided in the callback.
 		// Operators are expected to listen to `OracleRequest` events. This event contains all the required information to perform the request and provide back the result.
-		pub fn initiate_request(origin, operator: T::AccountId, spec_index: SpecIndex, data_version: DataVersion, data: Vec<u8>, fee: u32, callback: <T as Trait>::Callback) -> DispatchResult {
+		// REVIEW: Use a `BalanceOf` type for the fee instead of `u32` as shown here: https://substrate.dev/recipes/3-entrees/currency.html
+		pub fn initiate_request(origin, operator: T::AccountId, spec_index: SpecIndex, data_version: DataVersion, data: Vec<u8>, fee: BalanceOf<T>, callback: <T as Trait>::Callback) -> DispatchResult {
 			let who : <T as frame_system::Trait>::AccountId = ensure_signed(origin.clone())?;
 
-			ensure!(<Operators<T>>::exists(operator.clone()), Error::<T>::UnknownOperator);
-			ensure!(fee > 0, Error::<T>::InsufficientFee);
+			// REVIEW: This clone should not be necessary.
+			ensure!(<Operators<T>>::exists(&operator), Error::<T>::UnknownOperator);
+			// REVIEW: Should probably be at least `ExistentialDeposit`
+			ensure!(fee > BalanceOf::<T>::from(0), Error::<T>::InsufficientFee);
 
 			T::Currency::reserve(&who, fee.into())?;
 
 			let request_id = NextRequestIdentifier::get();
+			// REVIEW: This can overflow. You can make a maximum of `u64::max_value()` requests.
+			//         Default behavior for `u64` is to wrap around to 0, but you might want to
+			//         make this explicit.
+			//         I think using `wrapping_add` could be fine here, because it should be fine to
+			//         start at 0 when you reach `u64::max_value()`.
 			NextRequestIdentifier::put(request_id + 1);
 
+			// REVIEW: Is it intentional that requests are only valid during the current block?
 			let now = frame_system::Module::<T>::block_number();
+			// REVIEW: You might want to think about and document that your requests can be overwritten
+			//         as soon as the request id wraps around.
+			// REVIEW: Is the `Vec` intended for forward compatibility? It seems superfluous here.
 			Requests::<T>::insert(request_id.clone(), (operator.clone(), vec![callback], now, fee));
 
 			Self::deposit_event(RawEvent::OracleRequest(operator, spec_index, request_id, who, data_version, data, "Chainlink.callback".into(), fee));
@@ -158,7 +176,7 @@ decl_module! {
 		}
 
 		// The callback used to be notified of all Operators results.
-		// Only the Operator responsible for an identified request can notify back the result.
+		// Only the Operator responsible fo r an identified request can notify back the result.
 		// Result is then dispatched back to the originator's callback.
 		// The fee reserved during `initiate_request` is transferred as soon as this callback is called.
         fn callback(origin, request_id: RequestIdentifier, result: Vec<u8>) -> DispatchResult {
@@ -167,8 +185,17 @@ decl_module! {
 			ensure!(<Requests<T>>::exists(request_id.clone()), Error::<T>::UnknownRequest);
 			ensure!(<Requests<T>>::get(request_id.clone()).0 == who, Error::<T>::WrongOperator);
 
+			// REVIEW: `take` will do another `get` under the hood. Consider storing the result of
+			//         `get` above and then using `remove` to remove it from storage.
 			let (operator, callback, _, fee) = <Requests<T>>::take(request_id.clone());
 
+			// REVIEW: This does not make sure that the fee is payed. `repatriate_reserved` removes
+			//         *up to* the amount passed. [See here](https://substrate.dev/rustdocs/master/frame_support/traits/trait.ReservableCurrency.html#tymethod.repatriate_reserved)
+			//         Check `reserved_balance()` to make sure that the fee is payable via this method.
+			//         Maybe use a different payment method and check `total_balance()`. I don't know 
+			//         Substrate's Currency module well enough to tell.
+			// REVIEW: This happens *after* the request is `take`n from storage. Is that intended?
+			//         See ["verify first, write last"](https://substrate.dev/recipes/2-appetizers/1-hello-substrate.html#inside-a-dispatchable-call) motto.
 			T::Currency::repatriate_reserved(&who, &operator, fee.into())?;
 
 			// Dispatch the result to the original callback registered by the caller

substrate-node-example/runtime/src/example.rs

@@ -23,7 +23,7 @@ decl_module! {
             let parameters = ("get", "https://min-api.cryptocompare.com/data/pricemultifull?fsyms=ETH&tsyms=USD", "path", "RAW.ETH.USD.PRICE", "times", "100000000");
             let call: <T as Trait>::Callback = Call::callback(vec![]).into();
 
-            <chainlink::Module<T>>::initiate_request(origin, operator, 1, 0, parameters.encode(), 100, call.into())?;
+            <chainlink::Module<T>>::initiate_request(origin, operator, 1, 0, parameters.encode(), 100.into(), call.into())?;
 
             Ok(())
         }