SBP Milestone 1 Review

client/js-wasm/Cargo.toml

@@ -28,6 +28,7 @@ nolik-metadata = { path = "../metadata" }
 getrandom = { version = "0.2", default-features = false, features = ["js"] }
 crypto_box = { version = "0.8" }
 js-sys = "0.3.61"
+# SBP-M1 review: Please remove this commented dependency
 # anyhow = "1.0"
 
 

client/js-wasm/README.md

@@ -1,4 +1,5 @@
 ## 🚴 Usage
+SBP-M1 review: Please add a some module's description
 
 ### 🛠️ Build with `wasm-pack build`
 

client/js-wasm/src/lib.rs

@@ -136,6 +136,7 @@ pub fn new_encrypted_metadata(
 	let public_nonce = Nonce::<SalsaBox>::from_slice(public_nonce.as_slice());
 	let sender_pk = PublicKey::from(js_value_to_array::<KEY_SIZE>(sender_pk.into())?);
 
+	// SBP-M1 review: public nonce can be passed directly as the value is being dereferenced by the compiler immediately
 	let (meta, secret_nonce) = MessageMetadata::new_encrypted(
 		&origin,
 		&public_nonce,

client/metadata/src/lib.rs

@@ -25,6 +25,8 @@ mod ffi {
 		aead::{AeadCore, OsRng},
 		PublicKey, SalsaBox, SecretKey,
 	};
+
+	// SBP-M1 review: Please complete ToDos
 	// TODO: use scale codec instead of serde_json, also on a client
 	use serde::{Deserialize, Serialize};
 	use std::{

client/metadata/src/messages.rs

@@ -69,6 +69,7 @@ where
 	D: Deserializer<'a>,
 {
 	use serde::de::Error;
+	// SBP-M1 review: no need to borrow, direct string can be passed in decode function
 	String::deserialize(deserializer).and_then(|string| {
 		general_purpose::STANDARD
 			.decode(&string)

node/src/benchmarking.rs

@@ -95,6 +95,7 @@ impl frame_benchmarking_cli::ExtrinsicBuilder for TransferKeepAliveBuilder {
 		"transfer_keep_alive"
 	}
 
+	// SBP-M1 review: No need to use into() for conversion in self.value, as value is u128 already
 	fn build(&self, nonce: u32) -> std::result::Result<OpaqueExtrinsic, &'static str> {
 		let acc = Sr25519Keyring::Bob.pair();
 		let extrinsic: OpaqueExtrinsic = create_benchmark_extrinsic(
@@ -160,6 +161,7 @@ pub fn create_benchmark_extrinsic(
 	);
 	let signature = raw_payload.using_encoded(|e| sender.sign(e));
 
+	// SBP-M1 review: redundant clone, can be removed
 	runtime::UncheckedExtrinsic::new_signed(
 		call.clone(),
 		sp_runtime::AccountId32::from(sender.public()).into(),

node/src/rpc.rs

@@ -45,6 +45,7 @@ where
 	let mut module = RpcModule::new(());
 	let FullDeps { client, pool, deny_unsafe } = deps;
 
+	// SBP-M1 review: No need to use clone for pool,
 	module.merge(System::new(client.clone(), pool.clone(), deny_unsafe).into_rpc())?;
 	module.merge(TransactionPayment::new(client).into_rpc())?;
 

node/src/service.rs

@@ -147,6 +147,7 @@ pub fn new_partial(
 	})
 }
 
+// SBP-M1 review: Try to deal with static memory (if possible), &str can be incorporated instead of &String
 fn remote_keystore(_url: &String) -> Result<Arc<LocalKeystore>, &'static str> {
 	// FIXME: here would the concrete keystore be built,
 	//        must return a concrete type (NOT `LocalKeystore`) that

pallets/nolik/README.md

@@ -1,6 +1,7 @@
 # Pallet Nolik
 A [Substrate](https://substrate.io) FRAME pallet for sending encrypted messages between blockchain accounts
 
+SBP-M1 review: Typo should be fixed (eg: echange)
 ## Overview
 Nolik is a protocol for secure and verifiable data echange between parties that do not trust each other.
 It is designed to connect people without any form of censorship or third-party control.
@@ -44,4 +45,4 @@ Required parameters
 The main functionality is covered by unit tests.
 To run the tests use the command:
 
-`cargo test`
+`cargo test`

pallets/nolik/src/lib.rs

@@ -69,23 +69,28 @@ pub mod pallet {
 		///   bytes so no encoding and no heap allocation is needed prior to putting the message to
 		///   off-chain storage.
 		#[pallet::call_index(0)]
+		// SBP-M1 review: Implement benchmarking and use benchmarked weight
 		#[pallet::weight(10_000)]
 		pub fn send_message(
 			origin: OriginFor<T>,
 			metadata: MessageMetadata,
+			// SBP-M1 review: BoundedVec should be used to improve security
 			message: Vec<u8>,
 		) -> DispatchResult {
 			let account = ensure_signed(origin)?;
 			Self::check_message(&message, &metadata)?;
 
 			let counter = MessageCounter::<T>::get();
+
+			// SBP-M1: Can be simplified like this `counter.checked_add(1).ok_or(<Error<T>>::MessageCounterOverflow)?`
 			let (counter, overflowed) = counter.overflowing_add(1);
 			// u128 should not overflow, practically impossible
 			if overflowed {
 				Err(<Error<T>>::MessageCounterOverflow)?;
 			}
 
 			let key = Self::derived_key(&account, counter - 1);
+			// SBP-M1 review: please remove commented code
 			// frame_support::log::info!("The offchain key !!! {:02x?}", key);
 
 			// save message to offchain storage