Add market pallet #1119
Add market pallet #1119
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let owner = asset_owner::<T>(); | ||
| let owner_origin: <T as frame_system::Config>::RuntimeOrigin = RawOrigin::Signed(owner.clone()).into(); | ||
| let price_asset = add_asset::<T>(); | ||
| let product = Pallet::<T>::create_new_product(owner.clone(), |
Check failure
Code scanning / clippy
redundant clone Error
| amount: Balance, | ||
| ) -> DispatchResultWithPostInfo { | ||
| let who = ensure_signed(origin)?; | ||
| let product = Products::<T>::get(&product_id).ok_or(Error::<T>::ProductNotFound)?; |
Check failure
Code scanning / clippy
the borrowed expression implements the required traits Error
| #[pallet::pallet] | ||
| #[pallet::generate_store(pub(super) trait Store)] | ||
| #[pallet::storage_version(STORAGE_VERSION)] | ||
| #[pallet::without_storage_info] |
|
|
||
| #[pallet::hooks] | ||
| impl<T: Config> Hooks<T::BlockNumber> for Pallet<T> { | ||
| fn on_initialize(now: T::BlockNumber) -> Weight { |
| pub fn buy( | ||
| origin: OriginFor<T>, | ||
| product_id: AssetIdOf<T>, | ||
| amount: Balance, |
There was a problem hiding this comment.
As I see, the product has precision 0. It means amount is a count of products. Balance type looks strange for this case, the balance can have precision (e.g. 0.001).
I suggest count: u64 or something like this
| #[pallet::event] | ||
| #[pallet::generate_deposit(pub(super) fn deposit_event)] | ||
| pub enum Event<T: Config> { | ||
| ProductRegistered { asset_id: AssetIdOf<T> }, |
There was a problem hiding this comment.
I think we need Buy event as well. It can make easier some search in indexer when it is necessary.
| impl<T: Config> Pallet<T> { | ||
| #[pallet::call_index(0)] | ||
| #[pallet::weight(<T as Config>::WeightInfo::create_product())] | ||
| pub fn create_product( |
There was a problem hiding this comment.
Pelase add descriptions for extrinsics
| Extension::RequiredProduct(product_id, _min_amount) => { | ||
| ensure!( | ||
| !disallowed_products.contains(product_id), | ||
| Error::<T>::AmbigiousProductRequirements | ||
| ); | ||
| ensure!( | ||
| required_products.insert(*product_id), | ||
| Error::<T>::AmbigiousProductRequirements | ||
| ); | ||
| ensure!( | ||
| Products::<T>::contains_key(product_id), | ||
| Error::<T>::ProductNotFound | ||
| ); | ||
| } | ||
| Extension::DisallowedProduct(product_id) => { |
There was a problem hiding this comment.
I think we have to check that product_id in RequiredProduct & DisallowedProduct is exactly a product.
Because now it can be just a regular asset.
On the other hand, if we want to allow use all assets here (regular, NFT, etc.) we need to rename ...Product into ...Asset.
|
|
||
| #[derive(RuntimeDebug, Encode, Decode, TypeInfo, Clone, PartialEq, Eq)] | ||
| pub struct Product<BlockNumber, AssetId> { | ||
| price_asset: AssetId, |
There was a problem hiding this comment.
| price_asset: AssetId, | |
| quote_asset: AssetId, |
|
|
||
| #[derive(RuntimeDebug, Encode, Decode, TypeInfo, Clone, PartialEq, Eq)] | ||
| pub struct Product<BlockNumber, AssetId> { | ||
| price_asset: AssetId, |
There was a problem hiding this comment.
What kind of assets are allowed for exchange? All assets? Just regular assets? Can user define NFT or Soulbound asset for exchange?
Looks like we need to check this asset.
| @@ -103,6 +103,7 @@ vested-rewards-runtime-api = { path = "../pallets/vested-rewards/runtime-api", d | |||
| xor-fee = { path = "../pallets/xor-fee", default-features = false } | |||
| xst = { path = "../pallets/xst", default-features = false } | |||
| xst-benchmarking = { path = "../pallets/xst/benchmarking", default-features = false, optional = true } | |||
| market = { path = "../pallets/market", default-features = false, optional = true } | |||
There was a problem hiding this comment.
It is better to keep the alphabetical order
| // OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, | ||
| // STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE | ||
| // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
|
|
There was a problem hiding this comment.
It is better to prevent the running of tests without wip flag
| #![cfg(feature = "wip")] // Market | |
No description provided.