Review Basilisk (SBP Milestone 3) #113

apopiak · 2021-07-25T17:11:26Z

Review of the Basilisk runtime and included pallets for the Substrate Builders' Program Milestone 3.

Below is a summary of what I consider the most important points:

Runtime

make sure to generate weights
what's your dust management strategy?
you seem to have copied the Hydra pallets which IMO adds maintenance burden

pallet-asset-registry

don't use unwrap in the runtime
suggestion: check the genesis config for sanity

pallet-exchange

I don't think you cover the worst case for the on_finalize weight

pallet-lbp

some confusion about naming/semantics of pausing and pool activity

pallet-nft

mint is not marked #[transactional] but should be
benchmarking does not assume worst case

pallet-transaction-multi-payment

benchmarking does not assume worst case

pallets/transaction-multi-payment/src/lib.rs

jak-pan · 2021-07-26T19:29:54Z

Review of the Basilisk runtime and included pallets for the Substrate Builders' Program Milestone 3.

Below is a summary of what I consider the most important points:

Runtime

make sure to generate weights

In progress

what's your dust management strategy?

We are developing pallet-duster #104 please feel free to comment / review on it @enthusiastmartin if ready.

you seem to have copied the Hydra pallets which IMO adds maintenance burden

We did this because basilisk is being audited and we'll be removing them from HydraDX soon.

Others 👍🏻 on it.

jak-pan · 2021-07-26T19:35:06Z

runtime/src/lib.rs

@@ -446,6 +450,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime {
 	type OnValidationData = ();
 	type SelfParaId = ParachainInfo;
 	type OutboundXcmpMessageSource = ();
+	// REVIEW: I'd understand holding off on XCMP, but you're not even handling DMPs, why?


It's wip #114

jak-pan · 2021-07-26T19:35:42Z

runtime/src/lib.rs

 	type WeightInfo = ();
 }

 parameter_types! {
 	pub const CouncilMotionDuration: BlockNumber = 5 * DAYS;
 	pub const CouncilMaxProposals: u32 = 20;
+	// REVIEW: What's the use of having a council of one?


We'll be increasing it after the chain runs.

jak-pan · 2021-07-26T19:39:19Z

pallets/lbp/src/lib.rs

@@ -504,7 +505,9 @@ pub mod pallet {

 				ensure!(pool.paused, Error::<T>::PoolIsNotPaused);

-				ensure!(Self::is_after_sale(&pool), Error::<T>::CannotUnpauseEndedPool);
+				// REVIEW: You want to ensure the pool is running, no? Or do you want to allow


we want to allow paused start -> docs?

sure, but the function did sth else AFAICT

enthusiastmartin · 2021-08-09T09:53:56Z

pallets/asset-registry/src/lib.rs

+			// REVIEW: You probably want to make sure the genesis config is sane?
+			assert_ne!(self.core_asset_id, self.next_asset_id, "core asset id needs to be unique");
+			// REVIEW: maybe even:
+			assert!(self.core_asset_id < self.next_asset_id, "core asset id needs to be unique");
 			CoreAssetId::<T>::put(self.core_asset_id);
 			NextAssetId::<T>::put(self.next_asset_id);
+			// REVIEW: Maybe track them in a btree-set? Might be overkill, not sure.
 			self.asset_ids.iter().for_each(|(name, asset_id)| {
+				// REVIEW: maybe:
+				assert!(asset_id < &self.next_asset_id, "asset ids need to be unique");


Yes, this pallet had the refactor coming for quite some time.

And now, we're completely reworking our asset-registry. It will include better handling of assets in genesis config too.

enthusiastmartin · 2021-08-24T10:42:59Z

runtime/Cargo.toml

+# REVIEW: Seems like you copied the pallets from HydraDX node. Seems like a bad idea as it increases
+#   the maintenance burden.


This has been changed in HydraDX node repo. And now, the hydra repo uses pallets from Basilisk to remove the maintenance burden.

enthusiastmartin · 2021-08-24T10:45:39Z

runtime/src/lib.rs

@@ -312,6 +312,7 @@ impl pallet_timestamp::Config for Runtime {
 }

 parameter_types! {
+	// REVIEW: How do you avoid dust?


Dust handling is done by pallet-duster and pallet-offchain-duster.

enthusiastmartin · 2021-08-24T10:45:46Z

runtime/src/lib.rs

@@ -373,6 +376,7 @@ impl pallet_sudo::Config for Runtime {
 }

 parameter_type_with_key! {
+	// REVIEW: How do you plan on handling dust?


Dust handling is done by pallet-duster and pallet-offchain-duster.

enthusiastmartin · 2021-09-08T15:15:38Z

pallets/exchange/src/lib.rs

+			// REVIEW: I think you can avoid the allocation of the vec here if you use `take_while`
+			// https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.take_while
+			// on an iterator of the slice.


Thanks.
I finally refactored the matching part of the algorithm - using also your take_while suggestion. Also removed the unnecessary vec copy.

PR Here

enthusiastmartin · 2021-09-24T18:06:03Z

pallets/exchange/benchmarking/src/lib.rs

+		// REVIEW: I don't think one sell covers the worst-case on_finalize overhead.
+		// I would expect `resolve_matched_intentions` not to be called with just one intention
+		// because it's based on the matching. The on_finalize overhead of one sell should probably
+		// be based on max-buy-intentions already in the queue and vice-versa for one buy.


we will be refactoring the algorithm in very near future so that would be updated too. so leaving as is for now.

enthusiastmartin · 2021-09-24T18:06:54Z

All review comments have been addressed and either fixed/changed or issue has been created to address it later.

apopiak added 8 commits July 25, 2021 19:06

review transaction-multi-payment

4fb3a42

review runtime

bc55541

review pallet-nft

becc52c

review transaction-multi-payment benchmarks

8a9b5e3

review pallet-nft benchmarking

7a04d18

review pallet-lbp

57be7ce

review pallet-exchange

c53cf61

review pallet-asset-registry

97687e6

enthusiastmartin reviewed Jul 26, 2021

View reviewed changes

pallets/transaction-multi-payment/src/lib.rs Show resolved Hide resolved

jak-pan reviewed Jul 26, 2021

View reviewed changes

review pallet-xyk

3e8df34

enthusiastmartin reviewed Aug 9, 2021

View reviewed changes

enthusiastmartin reviewed Aug 24, 2021

View reviewed changes

enthusiastmartin reviewed Sep 8, 2021

View reviewed changes

enthusiastmartin reviewed Sep 24, 2021

View reviewed changes

enthusiastmartin closed this Sep 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review Basilisk (SBP Milestone 3) #113

Review Basilisk (SBP Milestone 3) #113

apopiak commented Jul 25, 2021 •

edited

jak-pan commented Jul 26, 2021 •

edited by enthusiastmartin

Runtime

jak-pan Jul 26, 2021

jak-pan Jul 26, 2021

jak-pan Jul 26, 2021

apopiak Jul 27, 2021

enthusiastmartin Aug 9, 2021

enthusiastmartin Aug 24, 2021

enthusiastmartin Aug 24, 2021

enthusiastmartin Aug 24, 2021

enthusiastmartin Sep 8, 2021

enthusiastmartin Sep 24, 2021

enthusiastmartin commented Sep 24, 2021

		# REVIEW: Seems like you copied the pallets from HydraDX node. Seems like a bad idea as it increases
		# the maintenance burden.

Review Basilisk (SBP Milestone 3) #113

Review Basilisk (SBP Milestone 3) #113

Conversation

apopiak commented Jul 25, 2021 • edited

Runtime

pallet-asset-registry

pallet-exchange

pallet-lbp

pallet-nft

pallet-transaction-multi-payment

jak-pan commented Jul 26, 2021 • edited by enthusiastmartin

Runtime

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enthusiastmartin commented Sep 24, 2021

apopiak commented Jul 25, 2021 •

edited

jak-pan commented Jul 26, 2021 •

edited by enthusiastmartin