feat: battlepass #120

FiberMan · 2022-12-15T11:48:09Z

No description provided.

battlepass/src/lib.rs

2075 · 2022-12-15T12:07:54Z

battlepass/src/lib.rs

+	#[pallet::getter(fn get_battlepass)]
+	pub(super) type Battlepasses<T: Config> = StorageMap<_, Blake2_128Concat, T::Hash, Battlepass<T::Hash, T::AccountId, T::BlockNumber, String<T>>, OptionQuery>;
+
+	/// Number of battlepasses per organization.


is it the number of claimed passes ?

No, it's the total number of created battlepasses per org.

so created not claimed? what is the difference? i think the pass is minted when you claim, so claimed==created.

no, it's a different entity, as @vovacha explained here.

you mean what @vayesy wrote? or do you reference something else?

sorry, yes, I meant @vayesy.

battlepass/src/lib.rs

2075 · 2022-12-15T12:13:34Z

battlepass/src/lib.rs

+	#[pallet::call]
+	impl<T: Config> Pallet<T> {
+		#[pallet::weight(0)]
+		pub fn create_battlepass(


if you put the transactional part into the extrinsic fn, you may need to double the code unneccessarily as we need a way to add a battlepass via oracle, therefore who, pass, account where who is permitted oracle and account is the user to add. also we need to store respective discord id somewhere otherwise we cannot map it,.

I think there is slight confusion in terminology.
We name two entities the same way: a) general entity, which is created by organization and represents event to participate for the users; b) entity, which users purchase to gain access to participate in the event.
Particularly this extrinsic is used to create a) entity type. So we don't need to store discord id here and extend functionality for the oracle. Those will be applied for working with b) entity type.

battlepass is the general seasonal wrapper for achievements which result in scores and which unlock claiming rewards. i have written a doc for it

pls check here @vayesy @FiberMan @vovacha https://github.com/gamedaoco/gamedao-protocol/pull/120/files#diff-46df5e399469d42e3bf99a2d701851c5c5ca55ac2897b4a0bedf423ef03d94ae

can you add the proper names for those entities and structs and show where they are stored? @vayesy

vovacha · 2022-12-16T07:58:10Z

battlepass/src/types.rs

+	pub org_id: Hash,
+	pub name: BoundedString,
+	pub cid: BoundedString,
+    pub state: BattlepassState,


Usually, we have separate storage for the state (ex. MemberStates, OrgStates). I'm not sure what ActiveBattlepassByOrg is, but storing the state in the struct and storing it in the separate storage is suboptimal since you need to update both.

Will create storage for states, like battlepass_id -> BattlepassState
ActiveBattlepassByOrg - is to check which is a currently active battlepass for the Org (org_id -> active battlepass_id). So the Active state will be stored in two places, but it will bring more benefits in terms of storage and usability, I suppose.

you can query the active battlepass either via the graph as a client or by filtering for active pass as a first class citizen (the pallet), what is the usecase for the pallet? it would only operate per id on a specific battlepass and reject if the operation is not allowed, e.g. not active, not owner

e.g. org -> vec<battlepass>

Example of use case: upon creation of new Battlepass for the Org, we need to check if there is an Active Battlepass (current season) already. Org may have multiple Battlepasses, but only one can be Active. So having a mapping org -> active battlepass is more convenient and faster than working with vec.

battlepass/src/lib.rs

2075 · 2022-12-16T15:42:44Z

battlepass/src/lib.rs

+			ensure!(T::Control::is_org_active(&org_id), Error::<T>::OrgUnknown);
+			// check if user is a member of organization
+			ensure!(T::Control::is_org_member_active(&org_id, &claimer), Error::<T>::NotMember);
+			// check if Org has active battlepass


if you look to the two checks above maybe we should introduce the same for battlepass, like T::Battlepass::is_battlepass_active ?

Here we need to get the active battlepass, not just check if it's active.

battlepass/src/lib.rs

2075 · 2022-12-16T15:45:28Z

battlepass/src/types.rs

+pub enum BattlepassState {
+	Draft = 0,
+	Active = 1,
+	Closed = 2,


Closed => Ended || 'Inactive' ?
also need Locked

I also tend to capitalize state, like DRAFT ACTIVE

maybe Closed -> Finished? And the extrinsic accordingly finish_battlepass?
Why do we need Locked? @2075

battlepass/src/lib.rs

battlepass/src/types.rs

battlepass/src/lib.rs

vovacha · 2022-12-16T17:05:24Z

battlepass/src/lib.rs

+				None, 												// royalty_amount
+				BoundedVec::truncate_from(b"NFT meta".to_vec()), 	// metadata 			TODO: what should be here?
+				false, 												// transferable
+				None												// resources


I don't like these inline comments.
Here are some standards for documentation of the Rust project.

I didn't find any info about inline comments in the standards.
Did you mean it's better to describe struct fields on the top of the struct? Like:

/// Battlepass struct /// /// `collection_id`: Collection that will store all claimed Battlepass-NFTs pub struct Battlepass...

battlepass/src/types.rs

vovacha · 2022-12-16T17:09:25Z

battlepass/src/types.rs

+
+#[derive(Encode, Decode, Default, PartialEq, Eq, TypeInfo, MaxEncodedLen)]
+pub struct BattlepassNft {
+	pub collection_id: u32,


Maybe I don't know the details here, but there should be a collection identifier in the rmrk NFT object. For me, this struct looks redundant, but feel free to ignore my comment.

agree, should be stored maybe in battlepass metadata as hash?

Agree.
Added battlepass_id (hash) to metadata field of minted NFT.
Storage ClaimedBattlepasses: (AccountId, BattlepassId) -> NftId will be used to quickly validate if Battlepass NFT was claimed by user.

vovacha · 2022-12-16T17:17:38Z

battlepass/src/lib.rs

+	#[pallet::generate_deposit(pub(super) fn deposit_event)]
+	pub enum Event<T: Config> {
+		/// New BattlePass created
+		BattlepassCreated {


I'd suggest using "Created", "Claimed", "Closed" instead. Like we have for Flow. Since the subject is always the same - Battlepass.

There will be other entities in the pallet, like Quests, Achievements, etc which will have similar events.
Of course, we may put them in separate pallets, but they are related only to Battlepass, so I'm not sure if we should do so.
WDYT @vovacha , @vayesy , @2075 ?

vovacha · 2022-12-16T17:28:08Z

battlepass/src/lib.rs

+	/// ActiveBattlepassByOrg: map Hash => Hash
+	#[pallet::storage]
+	#[pallet::getter(fn get_active_battlepass)]
+	pub type ActiveBattlepassByOrg<T: Config> = StorageMap<_, Blake2_128Concat, T::Hash, T::Hash, OptionQuery>;


There is only one usage of this storage and it's avoiding duplicates. And it's possible to achieve this with the existing Battlepass storage if you do the next steps:

remove created and updated from the Battlepass struct, since indexer nows this information it's redundant here

this way you'll have a unique hash of the Battlepass without any variables like BlockNumber

check if Battlepass doesn't contain a hash

Good idea. But in this case, we need to keep in mind to recreate all Battlepasses if something changed in Battlepass struct.
And with this approach, we can only check if the same Battlepass exists, but we need to check if there is an active battlepass.

vovacha · 2022-12-16T18:06:01Z

battlepass/src/lib.rs

+	///
+	/// BattlepassStates: map Hash => BattlepassState
+	#[pallet::storage]
+	#[pallet::getter(fn get_battlepass_state)]


We decided not to use getters. But maybe it's not actual anymore. WDYT? @vayesy

Thumbs up mean not use getters (why not then?) or not actual anymore? :)

vovacha · 2022-12-16T18:23:40Z

battlepass/src/lib.rs

+		#[pallet::weight(0)]
+		pub fn close_battlepass(
+			origin: OriginFor<T>,
+			org_id: T::Hash,


Why use org_id here instead of battlepass_id.

vovacha · 2022-12-16T18:23:51Z

battlepass/src/lib.rs

+		pub fn claim_battlepass(
+			origin: OriginFor<T>,
+			org_id: T::Hash,
+		) -> DispatchResult {


Why use org_id here instead of battlepass_id.

would work though

claim the active battlepass for an org would require this

or param is optional org or battlepass id

agree it is cleaner to use battlepass id

vovacha · 2022-12-16T18:24:23Z

battlepass/src/lib.rs

+			ensure!(T::Control::is_org_member_active(&org_id, &claimer), Error::<T>::NotMember);
+			// check if Org has active battlepass
+			let battlepass_id = Self::get_active_battlepass(org_id).ok_or(Error::<T>::BattlepassUnknown)?;
+			// check if Battlepass already claimed


If you'd use battlepass_id as an extrinsic param, you can remove this line.

2075 · 2022-12-16T19:22:12Z

would recommend to use ended vs closed in general, as battlepass is time based

* Remove unused migration. (#111) * Remove unused migration. * Remove empty migration.rs files * Fix tests. * Add M2 review comments * USE ORML dependencies from github instead of local version * fix: rm warnings * M2 review fixes (#115) * feat: update member state (#119) * feat: update orml references to git * feat: add update member ext * feat: update missing member state * feat: update event, api * Updated extrinsic. Added benchmarks Co-authored-by: vasylenko-yevhen <[email protected]> * feat: battlepass (#120) * Create Battlepass pallete * docs: add readme * Update extrinsics. * Add RMRK integration, mint Battlepass NFTs. Co-authored-by: 2075 <[email protected]> * Allow users to join to prime type org (#121) * Create Battlepass pallete * Add Battlepass activation, refactor. * Update lib.rs (#123) * add levels and rewards to battlepass. (#124) * Unit tests for Battlepass (#125) * Add wallet for battlepass Bot * feat: change root_or_governance for approval * feat: benchmarks for battlepass * feat: bump substrate version from 0.9.28 to 0.9.36 * Add extrinsic descriptions for Battlepass. * feat: Update Battlepass and Reward (#130) * feat:tests for update_battlepass and update_reward * feat: new benchmarks for battlepass. * feat: extend bot privileges * feat: claim battlepass by bot * feat: add, remove levels by bot * update benchmarks * feat: use unique NFT IDs * feat: remove ability to claim battlepass for self * allows non org members to participate in battlepass * finish tests, update weights * allows root account to add bot account to the battlepass * Correctly validate `add_bot` caller * fix: update_battlepass should update any number of fields * Remove RMRK * fix: should create battlepass if there is active one * feat: add option for name and cid (#142) * feat: add option for name and cid * feat: update tests * feat: add mock, update test * feat: fix types * feat: rm unused * feat: update readme, benchmarking * Fix type --------- Co-authored-by: vasylenko-yevhen <[email protected]> * feat: bump substrate version from 0.9.36 to 0.9.38 (#141) * feat: bump substrate version from 0.9.36 to 0.9.38 * bump protocol to 1.3.0 * use CID as collection metadata * Revert "feat: bump substrate version from 0.9.36 to 0.9.38 (#141)" (#144) This reverts commit d084aca. * Update substrate v36 to v38 (#145) * feat: bump substrate version from 0.9.36 to 0.9.38 * bump protocol to 1.3.0 * use CID as collection metadata --------- Co-authored-by: FiberMan <[email protected]> * fix: update_reward should update any number of fields (#146) * feat: allow bot to update the battlepass * feat: use provided CID as a metadata for NFT. * address comments from SBP3 review * Substrate upgrade 0.9.38 to 0.9.39 * Substrate upgrade 0.9.39 to 0.9.40 * remove deprecated macros * replace pallet_uniques with pallet_nfts (#152) * fix(battlepass): update permissions (#153) --------- Co-authored-by: Yura <[email protected]> Co-authored-by: Daniel Bigos <[email protected]> Co-authored-by: vasylenko-yevhen <[email protected]> Co-authored-by: 2075 <[email protected]>

Create Battlepass pallete

750c14a

FiberMan changed the base branch from release-1.1.2 to release-1.2.0 December 15, 2022 11:50

vovacha marked this pull request as draft December 15, 2022 12:01

2075 reviewed Dec 15, 2022

View reviewed changes

battlepass/src/lib.rs Show resolved Hide resolved

2075 reviewed Dec 15, 2022

View reviewed changes

battlepass/src/lib.rs Show resolved Hide resolved

2075 reviewed Dec 15, 2022

View reviewed changes

battlepass/src/lib.rs Show resolved Hide resolved

2075 reviewed Dec 15, 2022

View reviewed changes

battlepass/src/lib.rs Show resolved Hide resolved

2075 reviewed Dec 15, 2022

View reviewed changes

vayesy assigned FiberMan Dec 15, 2022

docs: add readme

b635906

vovacha reviewed Dec 16, 2022

View reviewed changes

battlepass/src/lib.rs Outdated Show resolved Hide resolved

FiberMan added 2 commits December 16, 2022 11:06

Update extrinsics.

1203772

Add RMRK integration, mint Battlepass NFTs.

92f3d02

FiberMan mentioned this pull request Dec 16, 2022

Create battlepass pallet playzero/zero-network#227

Closed

2 tasks

2075 changed the title ~~[WIP] Create Battlepass pallet~~ feat: battlepass Dec 16, 2022

2075 reviewed Dec 16, 2022

View reviewed changes

battlepass/src/lib.rs Show resolved Hide resolved

2075 reviewed Dec 16, 2022

View reviewed changes

FiberMan marked this pull request as ready for review December 16, 2022 16:51

vovacha suggested changes Dec 16, 2022

View reviewed changes

vovacha reviewed Dec 16, 2022

View reviewed changes

vovacha suggested changes Dec 16, 2022

View reviewed changes

2075 merged commit e6b8f77 into release-1.2.0 Dec 16, 2022

vayesy mentioned this pull request Dec 16, 2022

Release 1.2.0 #113

Merged

12 tasks

FiberMan mentioned this pull request Dec 20, 2022

Enhancements to battlepass pallet. #122

Merged

feat: battlepass #120

feat: battlepass #120

Conversation

FiberMan commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2075 Dec 16, 2022 • edited Loading

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

2075 Dec 16, 2022 • edited Loading

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

2075 Dec 16, 2022 • edited Loading

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

vovacha Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

FiberMan Dec 19, 2022 • edited Loading

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

2075 commented Dec 16, 2022

2075 Dec 16, 2022 •

edited

Loading

2075 Dec 16, 2022 •

edited

Loading

2075 Dec 16, 2022 •

edited

Loading

vovacha Dec 16, 2022 •

edited

Loading

FiberMan Dec 19, 2022 •

edited

Loading