AstarNetwork / astar-frame

Core frame modules for Astar & Shiden network.
Other
58 stars 38 forks source link

Remove preapproval remnants #90

Closed Dinonard closed 1 year ago

Dinonard commented 1 year ago

Pull Request Summary

Removed deprecated pre-approval items from dapps-staking pallet and removed storage cleanup code.

Once Astar has been upgraded, we should ensure we update to latest astar-frame dependencies.

github-actions[bot] commented 1 year ago

Code Coverage

Package Line Rate Branch Rate Health
frame/block-reward/src 96% 0%
frame/dapps-staking/src 94% 0%
frame/custom-signatures/src 81% 0%
frame/xc-asset-config/src 88% 0%
chain-extensions/impls/dapps-staking/src 0% 0%
precompiles/utils/src 84% 0%
frame/pallet-xcm/src 81% 0%
frame/dapps-staking/src/pallet 90% 0%
frame/collator-selection/src 89% 0%
precompiles/utils/src/data 72% 0%
frame/vesting/src 87% 0%
chain-extensions/types/dapps-staking/src 0% 0%
precompiles/assets-erc20/src 91% 0%
precompiles/utils/macro/src 0% 0%
chain-extensions/trait/src 0% 0%
precompiles/dapps-staking/src 95% 0%
precompiles/sr25519/src 78% 0%
precompiles/utils/macro/tests 100% 0%
precompiles/xcm/src 80% 0%
primitives/xcm/src 88% 0%
precompiles/substrate-ecdsa/src 78% 0%
Summary 86% (8334 / 9696) 0% (0 / 0)

Minimum allowed line rate is 60%

Dinonard commented 1 year ago

@shunsukew please check & approve.

Usually if upgrade involves bumping up pallet version in storage, there should be a check at the beginning which will skip it if the upgrade was already done. Right now this is causing problems when I try to use try-runtime because this check will fail (see the code).

Usually we (Astar) include runtime upgrades as part of runtime directly. E.g. as here. That way it can easily be removed.

Both approaches are fine, in a sense, but there should be some guards so upgrade isn't executed twice if it's done directly in the pallet.

shunsukew commented 1 year ago

@shunsukew please check & approve.

Usually if upgrade involves bumping up pallet version in storage, there should be a check at the beginning which will skip it if the upgrade was already done. Right now this is causing problems when I try to use try-runtime because this check will fail (see the code).

Usually we (Astar) include runtime upgrades as part of runtime directly. E.g. as here. That way it can easily be removed.

Both approaches are fine, in a sense, but there should be some guards so upgrade isn't executed twice if it's done directly in the pallet.

I see. Thank you so much for teaching me. If runtime upgrade logic is included in frame side and managed properly with versioning, then I can imagine some external chains which enable astar-frame pallet (just in case) can benefit from it. If I were a such user, I would feel very grateful with it at least. I'll make sure to add some guards (checking upgrade already done or not. checking versioning in pallet storage for e.g?) in case with writing runtime upgrade in frame side. (I don't say it should necessarily be written in frame side though)

Dinonard commented 1 year ago

@shunsukew that's correct, if this was a pallet which was used by other projects, we'd need to manage upgrade versions and keep the code for some time so everyone has had the chance to upgrade.

Since we know which runtimes use this pallet though, we can usually delete the code quite fast.