Subscriptions pallet #147

TarekkMA · 2022-09-30T08:28:39Z

No description provided.

Extend SpacesInterface

olehmell · 2023-01-30T15:46:30Z

pallets/subscriptions/src/types.rs

+    /// The balance required to subscribe to a space.
+    pub subscription: Balance,
+
+    /// Determines if subscriptions for a space s disabled.


Suggested change

/// Determines if subscriptions for a space s disabled.

/// Determines if subscriptions for a space is disabled.

Suggested change

/// Determines if subscriptions for a space s disabled.

/// Determines if subscriptions for a space are disabled.

olehmell · 2023-01-30T16:13:20Z

pallets/subscriptions/src/lib.rs

+        }
+
+        #[pallet::weight(< T as Config >::WeightInfo::unsubscribe())]
+        pub fn unsubscribe(origin: OriginFor<T>, space_id: T::SpaceId) -> DispatchResult {


Idk a reason why we dont clean the storage after unsubscribing

olehmell · 2023-01-30T16:28:25Z

pallets/subscriptions/src/types.rs

+    pub disabled: bool,
+
+    /// The id of the role that will be granted for space subscriber.
+    pub role_id: RoleId,


Why do we not put a permissions array here and create a role in extrinsic?

Not here. Why should we store permissions set here if we'll store it in a role?
I think you meant to provide permissions set to an extrinsic, create a role using RolesInterface, and then put the role id here.

So, can we rewrite?

olehmell · 2023-01-30T16:29:38Z

pallets/subscriptions/src/lib.rs

+            granted_role: T::RoleId,
+            price: BalanceOf<T>,
+        },
+        UserUnSubscribed {


Suggested change

UserUnSubscribed {

UserUnsubscribed {

F3Joule · 2023-02-03T12:56:53Z

pallets/roles/src/functions.rs

+impl<T: Config> RolesInterface<RoleId, SpaceId, T::AccountId, SpacePermission, T::BlockNumber>
+    for Pallet<T>
+{
+    fn get_role_space(role_id: RoleId) -> Result<SpaceId, DispatchError> {


Suggested change

fn get_role_space(role_id: RoleId) -> Result<SpaceId, DispatchError> {

fn get_role_space_id(role_id: RoleId) -> Result<SpaceId, DispatchError> {

F3Joule · 2023-02-03T12:58:20Z

pallets/roles/src/functions.rs

+        Ok(role.space_id)
+    }
+
+    fn grant_role(account_id: T::AccountId, role_id: RoleId) -> DispatchResult {


We always use just account or a descriptive name for a specific account.

Suggested change

fn grant_role(account_id: T::AccountId, role_id: RoleId) -> DispatchResult {

fn grant_role(account: T::AccountId, role_id: RoleId) -> DispatchResult {

F3Joule · 2023-02-03T13:10:31Z

pallets/subscriptions/README.md

@@ -0,0 +1,11 @@
+# Subscriptions Pallet


@Yung-Beef plz check

F3Joule · 2023-02-03T13:14:18Z

pallets/subscriptions/src/mock.rs

+use std::borrow::Borrow;
+
+use codec::Decode;
+use frame_support::{
+    dispatch::{DispatchError, DispatchResult},
+    parameter_types,
+    traits::{Everything, Get},
+};
+use sp_core::H256;
+use sp_io::TestExternalities;
+use sp_runtime::{
+    testing::Header,
+    traits::{BlakeTwo256, IdentityLookup},
+};
+use sp_std::convert::{TryFrom, TryInto};
+
+use pallet_permissions::SpacePermission;
+use subsocial_support::{
+    traits::{RolesInterface, SpacesInterface},
+    Content,
+};


Remove unused imports.

F3Joule · 2023-02-03T13:15:30Z

pallets/subscriptions/src/mock.rs

+parameter_types! {
+    pub static get_space_owner__return: Result<AccountId, DispatchError> = Ok(AccountId::decode(&mut sp_runtime::traits::TrailingZeroInput::zeroes()).unwrap());
+    pub static create_space__return: Result<SpaceId, DispatchError> = Ok(101);
+}
+
+clearable_parameter_type!(pub static get_space_owner__space_id: SpaceId);
+clearable_parameter_type!(pub static create_space__owner: AccountId);
+clearable_parameter_type!(pub static create_space__content: Content);


This produces plenty of warnings. Why not to use mockall here?

F3Joule · 2023-02-03T13:47:28Z

pallets/subscriptions/src/types.rs

@@ -0,0 +1,30 @@
+use frame_support::pallet_prelude::*;


@Yung-Beef this file plz

F3Joule · 2023-02-03T13:51:53Z

pallets/subscriptions/src/types.rs

+    pub disabled: bool,
+
+    /// The id of the role that will be granted for space subscriber.
+    pub role_id: RoleId,


Not here. Why should we store permissions set here if we'll store it in a role?
I think you meant to provide permissions set to an extrinsic, create a role using RolesInterface, and then put the role id here.

F3Joule · 2023-02-03T13:53:20Z

runtime/Cargo.toml

@@ -36,6 +36,7 @@ pallet-roles = { path = '../pallets/roles', default-features = false }
 pallet-space-follows = { path = '../pallets/space-follows', default-features = false }


You forgot to put pallet-subscriptions to std

Yung-Beef · 2023-02-03T16:12:13Z

pallets/subscriptions/src/types.rs

+    /// The balance required to subscribe to a space.
+    pub subscription: Balance,
+
+    /// Determines if subscriptions for a space s disabled.


Suggested change

/// Determines if subscriptions for a space s disabled.

/// Determines if subscriptions for a space are disabled.

Yung-Beef · 2023-02-03T16:13:35Z

pallets/subscriptions/src/types.rs

+    /// Determines if subscriptions for a space s disabled.
+    pub disabled: bool,
+
+    /// The id of the role that will be granted for space subscriber.


Suggested change

/// The id of the role that will be granted for space subscriber.

/// The id of the role that will be granted for a space subscriber.

Yung-Beef · 2023-02-03T16:14:08Z

pallets/subscriptions/src/types.rs

+    pub role_id: RoleId,
+}
+
+/// Information about space subscriber.


Suggested change

/// Information about space subscriber.

/// Information about a space subscriber.

Yung-Beef · 2023-02-03T16:14:36Z

pallets/subscriptions/src/types.rs

+/// Information about space subscriber.
+#[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+pub struct SubscriberInfo<Balance, RoleId, BlockNumber> {
+    /// The block number at which the subscriptions became active.


Suggested change

/// The block number at which the subscriptions became active.

/// The block number at which a subscription became active.

Yung-Beef · 2023-02-03T16:14:44Z

pallets/subscriptions/src/types.rs

+    /// The block number at which the subscriptions became active.
+    pub subscribed_on: BlockNumber,
+
+    /// The balance paid for the subscriptions.


Suggested change

/// The balance paid for the subscriptions.

/// The balance paid for the subscription.

Yung-Beef · 2023-02-03T16:16:38Z

pallets/subscriptions/README.md

@@ -0,0 +1,11 @@
+# Subscriptions Pallet
+
+Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.


Suggested change

Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.

Subscribing to a space is similar to following a space, except subscribers will be granted a role in the space. This role can have any set of permissions that the space owner chooses to allow.

Yung-Beef · 2023-02-03T16:17:26Z

pallets/subscriptions/README.md

+
+Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.
+
+For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.


Suggested change

For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.

For the first version of this pallet, a subscription is just a one time payment, but in the future, we will allow recurring subscription payments.

Yung-Beef · 2023-02-03T16:18:20Z

pallets/subscriptions/README.md

+
+For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.
+
+The space owner at any time can disable this role, and sets a new subscription settings with a new role.


Suggested change

The space owner at any time can disable this role, and sets a new subscription settings with a new role.

The space owner can disable this role at any time, and create a new subscription type with a new role.

This seems pretty shitty for end users.. I pay today and can lose my role tomorrow? We'll need some disclaimers on the UI

Yung-Beef · 2023-02-03T16:19:29Z

pallets/subscriptions/README.md

+
+The space owner at any time can disable this role, and sets a new subscription settings with a new role.
+
+The space owner can change the subscription value and it will be effective for new subscribers. 


Suggested change

The space owner can change the subscription value and it will be effective for new subscribers.

At any time, the space owner can change the cost of subscribing for new subscribers.

Yung-Beef · 2023-02-03T16:19:53Z

pallets/subscriptions/README.md

+
+The space owner can change the subscription value and it will be effective for new subscribers. 
+
+Old subscribers can still enjoy the role they have been granted as long as the role is not disabled.


Suggested change

Old subscribers can still enjoy the role they have been granted as long as the role is not disabled.

Old subscribers can still enjoy their roles as long as those roles have not disabled.

siman · 2023-02-06T10:38:36Z

Task linked: CU-861m92u45 Release Subscriptions pallet

olehmell · 2023-02-06T12:05:32Z

pallets/subscriptions/src/types.rs

+    pub disabled: bool,
+
+    /// The id of the role that will be granted for space subscriber.
+    pub role_id: RoleId,


So, can we rewrite?

siman · 2023-02-17T07:15:50Z

Task linked: CU-861max1tq Fix subscription pallet according to reviews

siman changed the title ~~Subscription pallet~~ Subscriptions pallet Oct 6, 2022

TarekkMA and others added 3 commits October 19, 2022 14:39

SpacesInterface

cad2229

Add create_space_as_profile

01722b5

Extend SpacesInterface

Fix erros in tests

5b0d23e

TarekkMA force-pushed the tarekkma/subscription-pallet branch from 80e8049 to e472c50 Compare October 27, 2022 09:14

TarekkMA added 15 commits November 14, 2022 18:49

add subscription pallet

4982cb1

Apply suggestions

692bb90

Apply suggestions

c1c054b

Add description

0754db0

Add a readme file

6dedd23

Add unsubscribe

9f97631

rename pallet

e7228ed

remove Space from types name

3b82451

fix names

392df10

impl traits

c9ae843

add to runtime

87dcd56

benchmarking

edc1f05

benchmarking tests

db80514

update_subscription_settings test

f4fa056

mock params

446b3ab

TarekkMA force-pushed the tarekkma/subscription-pallet branch from c0c8428 to 446b3ab Compare November 14, 2022 16:49

F3Joule assigned TarekkMA Dec 27, 2022

F3Joule added help wanted Extra attention is needed question Further information is requested labels Dec 27, 2022

F3Joule requested review from F3Joule, siman and olehmell December 27, 2022 14:34

olehmell suggested changes Jan 30, 2023

View reviewed changes

F3Joule requested changes Feb 3, 2023

View reviewed changes

Yung-Beef reviewed Feb 3, 2023

View reviewed changes

olehmell approved these changes Feb 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions pallet #147

Subscriptions pallet #147

TarekkMA commented Sep 30, 2022

olehmell Jan 30, 2023

Yung-Beef Feb 3, 2023

olehmell Jan 30, 2023

olehmell Jan 30, 2023

F3Joule Feb 3, 2023

olehmell Feb 6, 2023

olehmell Jan 30, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

F3Joule Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

Yung-Beef Feb 3, 2023

siman commented Feb 6, 2023

olehmell Feb 6, 2023

siman commented Feb 17, 2023

	/// Determines if subscriptions for a space s disabled.
	/// Determines if subscriptions for a space is disabled.

	fn get_role_space(role_id: RoleId) -> Result<SpaceId, DispatchError> {
	fn get_role_space_id(role_id: RoleId) -> Result<SpaceId, DispatchError> {

	fn grant_role(account_id: T::AccountId, role_id: RoleId) -> DispatchResult {
	fn grant_role(account: T::AccountId, role_id: RoleId) -> DispatchResult {

		@@ -36,6 +36,7 @@ pallet-roles = { path = '../pallets/roles', default-features = false }
		pallet-space-follows = { path = '../pallets/space-follows', default-features = false }

	/// The id of the role that will be granted for space subscriber.
	/// The id of the role that will be granted for a space subscriber.

	/// Information about space subscriber.
	/// Information about a space subscriber.

	/// The block number at which the subscriptions became active.
	/// The block number at which a subscription became active.

	/// The balance paid for the subscriptions.
	/// The balance paid for the subscription.

		@@ -0,0 +1,11 @@
		# Subscriptions Pallet

		Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.

	Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.
	Subscribing to a space is similar to following a space, except subscribers will be granted a role in the space. This role can have any set of permissions that the space owner chooses to allow.


		Subscribing to a space is something similar to following a space, except subscribers will be granted a role for the space. This role can have any set of permissions space owner chooses to allow.

		For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.

	For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.
	For the first version of this pallet, a subscription is just a one time payment, but in the future, we will allow recurring subscription payments.


		For the first version of this pallet subscription is meant to be only one time payment. But in the future we will allow recurring subscription.

		The space owner at any time can disable this role, and sets a new subscription settings with a new role.

	The space owner at any time can disable this role, and sets a new subscription settings with a new role.
	The space owner can disable this role at any time, and create a new subscription type with a new role.


		The space owner at any time can disable this role, and sets a new subscription settings with a new role.

		The space owner can change the subscription value and it will be effective for new subscribers.

	The space owner can change the subscription value and it will be effective for new subscribers.
	At any time, the space owner can change the cost of subscribing for new subscribers.


		The space owner can change the subscription value and it will be effective for new subscribers.

		Old subscribers can still enjoy the role they have been granted as long as the role is not disabled.

	Old subscribers can still enjoy the role they have been granted as long as the role is not disabled.
	Old subscribers can still enjoy their roles as long as those roles have not disabled.

Subscriptions pallet #147

Are you sure you want to change the base?

Subscriptions pallet #147

Conversation

TarekkMA commented Sep 30, 2022

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

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

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

siman commented Feb 6, 2023

Choose a reason for hiding this comment

siman commented Feb 17, 2023