Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused migration. #111

Merged
merged 3 commits into from
Oct 27, 2022
Merged

Conversation

FiberMan
Copy link
Contributor

@FiberMan FiberMan commented Oct 20, 2022

  • remove hardcoded storage versions
  • remove migrations call from the hooks since it's not used
  • fix signal pallet tests

Test scenario:

  • build --release ok
  • build --benchmarks ok
  • make test ok

@FiberMan FiberMan requested review from vovacha and vayesy October 20, 2022 21:03
Copy link
Contributor

@vovacha vovacha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of having commented code in the repo.
Could you uncomment it and leave it as a template? So all future migrations will use this approach with the StorageVersion trait.

@FiberMan
Copy link
Contributor Author

I'm not a big fan of having commented code in the repo. Could you uncomment it and leave it as a template? So all future migrations will use this approach with the StorageVersion trait.

Tbh, I'm not a fan of having some code in repo that is not in use :)
We can restore it from GIT history if needed.
Removing unused files.

@FiberMan FiberMan requested a review from vovacha October 24, 2022 15:21
@vayesy
Copy link
Contributor

vayesy commented Oct 24, 2022

I can see we would still have issues with future migrations, which can be applied multiple times. Shouldn't this PR implement a solution for that by storing pallet version in the storage and deciding on migrations based on that value?

@FiberMan FiberMan force-pushed the Remove-migration-from-palletes branch from b6701ea to 2ee421b Compare October 24, 2022 21:12
@FiberMan
Copy link
Contributor Author

StorageVersion defaults to 0 when it is not present in the storage.
So on the next migration, we will need something like this:

  1. get on-chain storage version.
  2. apply migration which version is next to the one on chain.
  3. update on-chain storage version.

@vovacha
Copy link
Contributor

vovacha commented Oct 25, 2022

I can see we would still have issues with future migrations, which can be applied multiple times. Shouldn't this PR implement a solution for that by storing pallet version in the storage and deciding on migrations based on that value?

As far as I understand it's a global storage, not a pallet storage and it works like this:

use frame_support::traits::{GetStorageVersion, StorageVersion}

## Get Pallet's storage version
let on_chain_storage_version = <P as GetStorageVersion>::on_chain_storage_version();

## Do migrations
...

## Set the new pallet's storage version
StorageVersion::new(4).put::<P as GetStorageVersion>();

@vovacha
Copy link
Contributor

vovacha commented Oct 26, 2022

@FiberMan please, change the target branch from main to release.

@vayesy vayesy changed the base branch from main to release-1.1.3 October 27, 2022 16:19
@vayesy vayesy merged commit cde806b into release-1.1.3 Oct 27, 2022
@vayesy vayesy deleted the Remove-migration-from-palletes branch October 27, 2022 16:20
@vayesy vayesy mentioned this pull request Oct 27, 2022
12 tasks
@FiberMan FiberMan linked an issue Oct 31, 2022 that may be closed by this pull request
2075 added a commit that referenced this pull request May 1, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change pallet level storage versioning approach
3 participants