SBP Milestone 1 review

pallets/anonymity-mining/src/lib.rs

@@ -64,6 +64,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct. 
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -140,7 +144,13 @@ pub mod pallet {
 	#[pallet::getter(fn parameters)]
 	/// Details of the module's parameters
 	pub(super) type Parameters<T: Config<I>, I: 'static = ()> =
-		StorageValue<_, Vec<u8>, ValueQuery>;
+
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct. 
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
+	StorageValue<_, Vec<u8>, ValueQuery>;
 
 	#[pallet::storage]
 	#[pallet::getter(fn get_pool_weight)]

pallets/asset-registry/src/lib.rs

@@ -95,6 +95,10 @@ pub mod pallet {
 	}
 
 	#[pallet::pallet]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
@@ -256,6 +260,9 @@ pub mod pallet {
 		///
 		/// Emits 'Registered` event when successful.
 		#[pallet::weight(<T as Config>::WeightInfo::register())]
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		pub fn register(
 			origin: OriginFor<T>,
@@ -283,6 +290,9 @@ pub mod pallet {
 
 		// TODO: No tests
 		#[pallet::weight(<T as Config>::WeightInfo::update())]
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		pub fn update(
 			origin: OriginFor<T>,
@@ -329,6 +339,9 @@ pub mod pallet {
 		///
 		/// Emits `MetadataSet` event when successful.
 		#[pallet::weight(<T as Config>::WeightInfo::set_metadata())]
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		pub fn set_metadata(
 			origin: OriginFor<T>,
@@ -363,6 +376,9 @@ pub mod pallet {
 		///
 		/// Emits `LocationSet` event when successful.
 		#[pallet::weight(<T as Config>::WeightInfo::set_location())]
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		pub fn set_location(
 			origin: OriginFor<T>,

pallets/hasher/src/lib.rs

@@ -60,6 +60,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -106,6 +110,12 @@ pub mod pallet {
 	#[pallet::storage]
 	#[pallet::getter(fn parameters)]
 	/// Details of the module's parameters
+	/// 
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct. 
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub(super) type Parameters<T: Config<I>, I: 'static = ()> =
 		StorageValue<_, Vec<u8>, ValueQuery>;
 

pallets/key-storage/src/lib.rs

@@ -43,6 +43,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+		/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct. 
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 

pallets/linkable-tree/src/lib.rs

@@ -100,11 +100,22 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
 	#[pallet::config]
 	/// The module configuration trait.
+	///
+	/// HB Milestone Review 1
+	/// Is the tightly coupling `+ pallet_mt::Config<I>` needed in this case?
+	/// For example, you can get rid of `+ pallet_mt::Config<I>` and just import the  TreeInterface
+	/// from the pallet without requiring this inherince.
+	/// https://docs.substrate.io/build/pallet-coupling/
+
 	pub trait Config<I: 'static = ()>: frame_system::Config + pallet_mt::Config<I> {
 		/// The overarching event type.
 		type RuntimeEvent: From<Event<Self, I>>

pallets/mixer/src/lib.rs

@@ -101,6 +101,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+		/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct. 
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -240,7 +244,9 @@ pub mod pallet {
 			Self::deposit_event(Event::MixerCreation { tree_id });
 			Ok(().into())
 		}
-
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		#[pallet::weight(<T as Config<I>>::WeightInfo::deposit())]
 		pub fn deposit(
@@ -254,6 +260,9 @@ pub mod pallet {
 			Ok(().into())
 		}
 
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		#[pallet::weight(<T as Config<I>>::WeightInfo::withdraw())]
 		pub fn withdraw(

pallets/mt/src/lib.rs

@@ -86,6 +86,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -173,6 +177,11 @@ pub mod pallet {
 	/// The default hashes for this tree pallet
 	#[pallet::storage]
 	#[pallet::getter(fn default_hashes)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub(super) type DefaultHashes<T: Config<I>, I: 'static = ()> =
 		StorageValue<_, Vec<T::Element>, ValueQuery>;
 
@@ -382,6 +391,9 @@ impl<T: Config<I>, I: 'static> TreeInterface<T::AccountId, T::TreeId, T::Element
 	fn create(creator: Option<T::AccountId>, depth: u8) -> Result<T::TreeId, DispatchError> {
 		// Setting the next tree id
 		let tree_id = Self::next_tree_id();
+		// HB Milestone review 1:
+		// The "+=" might cause an overflow depending on the type provided on TreeId. I would change
+		// this operation to saturating_add()
 		NextTreeId::<T, I>::mutate(|id| *id += One::one());
 		// get unit of two
 		let two: T::LeafIndex = Self::two();
@@ -449,6 +461,9 @@ impl<T: Config<I>, I: 'static> TreeInterface<T::AccountId, T::TreeId, T::Element
 			*i = i.saturating_add(One::one()) % T::RootHistorySize::get()
 		});
 		CachedRoots::<T, I>::insert(id, root_index, hash);
+		// HB Milestone review 1:
+		// The "+=" might cause an overflow depending on the type provided on TreeId. I would change
+		// this operation to saturating_add() ( same as NextRootIndex mutator)
 		NextLeafIndex::<T, I>::mutate(id, |i| *i += One::one());
 
 		// return the root
@@ -473,7 +488,9 @@ impl<T: Config<I>, I: 'static> TreeInspector<T::AccountId, T::TreeId, T::Element
 				if cached_root == target_root {
 					return Ok(true)
 				}
-
+				// HB Milestone review 1:
+				// The "+=" might cause an overflow depending on the type provided on TreeId. I
+				// would change this operation to saturating_add()
 				temp += One::one();
 			}
 

pallets/relayer-registry/src/lib.rs

@@ -15,6 +15,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+	/// HB Milestone Review 1
+	/// The documentation for this pallet described other pallet than the relayer registry.
+
+
 //! # Identity Pallet
 //!
 //! - [`Config`]

pallets/signature-bridge/src/lib.rs

@@ -89,6 +89,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -147,6 +151,11 @@ pub mod pallet {
 	/// The parameter maintainer who can change the parameters
 	#[pallet::storage]
 	#[pallet::getter(fn maintainer)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub type Maintainer<T: Config<I>, I: 'static = ()> = StorageValue<_, Vec<u8>, ValueQuery>;
 
 	/// All whitelisted chains and their respective transaction counts

pallets/token-wrapper-handler/src/lib.rs

@@ -101,6 +101,9 @@ pub mod pallet {
 		StorageOverflow,
 	}
 
+	/// HB Milestone Review 1
+	/// All the calls from this pallet have the weights hardcoded to 195_000_000
+	/// Is that intentional?
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		/// Execute the wrapping fee proposal by calling the update_wrapping_fee

pallets/token-wrapper/src/lib.rs

@@ -89,6 +89,10 @@ pub mod pallet {
 	use frame_system::{ensure_signed, pallet_prelude::*};
 
 	#[pallet::pallet]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
@@ -146,11 +150,21 @@ pub mod pallet {
 	/// Fee recipient, account which will be receiving wrapping cost fee.
 	#[pallet::storage]
 	#[pallet::getter(fn fee_recipient)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub type FeeRecipient<T: Config> = StorageMap<_, Blake2_128Concat, Vec<u8>, T::AccountId>;
 
 	/// The proposal nonce used to prevent replay attacks on execute_proposal
 	#[pallet::storage]
 	#[pallet::getter(fn proposal_nonce)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub type ProposalNonce<T: Config> = StorageMap<_, Blake2_128Concat, Vec<u8>, T::ProposalNonce>;
 
 	#[pallet::event]

pallets/vanchor-handler/src/lib.rs

@@ -68,6 +68,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -121,6 +125,9 @@ pub mod pallet {
 	#[pallet::hooks]
 	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {}
 
+	/// HB Milestone Review 1
+	/// All the calls from this pallet have the weights hardcoded to 195_000_000
+	/// Is that intentional?
 	#[pallet::call]
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// This will be called by bridge when proposal to create a

pallets/vanchor-verifier/src/lib.rs

@@ -82,6 +82,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -131,6 +135,12 @@ pub mod pallet {
 	#[pallet::storage]
 	#[pallet::getter(fn parameters)]
 	/// Details of the module's parameters for different vanchor configurations
+	///
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
+	/// Please use BoundedVec insted Vec
 	pub(super) type Parameters<T: Config<I>, I: 'static = ()> =
 		StorageMap<_, Blake2_128Concat, (u8, u8), Vec<u8>, ValueQuery>;
 

pallets/vanchor/src/lib.rs

@@ -104,6 +104,10 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	/// HB Milestone Review 1
+	/// Macro without_storage_info should not be used any more since unbounded Vecs might interfeer
+	/// in the proof_size calculation of the new Weights v2 struct.
+	/// Please check: https://github.com/paritytech/substrate/issues/8629
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
@@ -119,6 +123,12 @@ pub mod pallet {
 		#[pallet::constant]
 		type PalletId: Get<PalletId>;
 
+		/// HB Milestone Review 1
+		/// I think the tighly coupling with the pallet `+ pallet_linkable_tree::Config<I>` can be
+		/// avoided: The LinkableTreeInterface can access to the LinkableTreeConfigration without
+		/// having to TreeInterface from the pallet without requiring this inheritance.
+		/// https://docs.substrate.io/build/pallet-coupling/
+
 		/// The tree type
 		type LinkableTree: LinkableTreeInterface<pallet_linkable_tree::LinkableTreeConfigration<Self, I>>
 			+ LinkableTreeInspector<pallet_linkable_tree::LinkableTreeConfigration<Self, I>>;
@@ -315,6 +325,11 @@ pub mod pallet {
 					T::ProposalNonce::from(ctr),
 				)
 				.map_err(|_| panic!("Failed to create vanchor"));
+				// HB Milestone review 1:
+				// The "+=" might cause an overflow depending on the type provided on TreeId. I
+				// would change this operation to saturating_add().
+				// This comment is more theorical and clean code since i'm not sure if there is
+				// going to be a case where the ctr variable might overflow on the GenesisBuild.
 				ctr += 1;
 			});
 		}
@@ -341,6 +356,9 @@ pub mod pallet {
 			Ok(().into())
 		}
 
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		#[pallet::weight(<T as pallet::Config<I>>::WeightInfo::transact())]
 		pub fn transact(
@@ -354,6 +372,9 @@ pub mod pallet {
 			Ok(().into())
 		}
 
+		// HB Milestone Review 1
+		// It is not necessary to declare the #[transactional] macro anymore since it is added by
+		// default. https://github.com/paritytech/substrate/pull/11431
 		#[transactional]
 		#[pallet::weight(<T as pallet::Config<I>>::WeightInfo::register_and_transact())]
 		pub fn register_and_transact(