Skip to content
This repository has been archived by the owner on Sep 28, 2023. It is now read-only.

dApps staking v3 - part 1 #155

Merged
merged 18 commits into from
Jun 14, 2023
Merged

dApps staking v3 - part 1 #155

merged 18 commits into from
Jun 14, 2023

Conversation

Dinonard
Copy link
Member

@Dinonard Dinonard commented May 11, 2023

Pull Request Summary

First PR that will introduce part of dApps staking v3 functionality.
Overall implementation will be broken down into multiple PRs to make it easier for reviewers.

Part 1

  • maintenance mode
  • basic operations for registering dApps
  • dApp unregistration, reward destination & owner manipulation
  • various struct & types to simplify the implementation
  • balance lock implementation

After receiving first review, will amend the comments and implement extensive unit tests.

Check list

  • dApp manipulation & balance lock implemented
  • first review received
  • added unit tests

@Dinonard Dinonard changed the title dApps staking v3 - inception dApps staking v3 - part 1 May 11, 2023
},
)?;

// TODO: might require some modification later on, like additional checks to ensure contract can be unregistered.

Choose a reason for hiding this comment

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

what are the scenarios in which contract can't be unregistered?

Copy link
Member Author

Choose a reason for hiding this comment

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

What should be avoided is unregistering a contract while leaving storage items behind, forever.
So later on, when PR progresses more, this TODO is a reminder to revisit the issue (I don't have a clear solution for this yet).

frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

Looks good in general!

I wonder why we are using DispatchResultWithPostInfo as the return type though there is no post info?

frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
@Dinonard
Copy link
Member Author

Looks good in general!

I wonder why we are using DispatchResultWithPostInfo as the return type though there is no post info?

Thanks!

For the DispatchResultWithPostInfo you're right that it's not needed. I just copied it from the previous pallet for the first extrinsic I added, then reused it afterwards 😁. Fixed it now.

@Dinonard Dinonard marked this pull request as ready for review June 12, 2023 08:29
@Dinonard
Copy link
Member Author

All functionality intended for this PR has been implemented, and it's ready for final review 🙏

Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just some small issues.

frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/test/mock.rs Outdated Show resolved Hide resolved
frame/dapp-staking-v3/src/lib.rs Show resolved Hide resolved
Co-authored-by: Shaun Wang <spxwang@gmail.com>
@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
chain-extensions/types/assets/src 0% 0%
chain-extensions/pallet-assets/src 0% 0%
frame/pallet-xcm/src 53% 0%
chain-extensions/types/xvm/src 0% 0%
frame/pallet-xvm/src 5% 0%
frame/pallet-xvm/src/pallet 8% 0%
precompiles/xvm/src 61% 0%
primitives/xcm/src 62% 0%
precompiles/sr25519/src 79% 0%
precompiles/substrate-ecdsa/src 78% 0%
frame/block-reward/src 80% 0%
frame/collator-selection/src 76% 0%
frame/contracts-migration/src 0% 0%
frame/xc-asset-config/src 50% 0%
precompiles/dapps-staking/src 93% 0%
precompiles/utils/src 69% 0%
precompiles/xcm/src 85% 0%
precompiles/assets-erc20/src 76% 0%
precompiles/utils/macro/tests 0% 0%
precompiles/utils/macro/src 0% 0%
frame/dapps-staking/src/pallet 87% 0%
frame/dapp-staking-v3/src/test 0% 0%
chain-extensions/xvm/src 0% 0%
chain-extensions/types/dapps-staking/src 0% 0%
frame/dapp-staking-v3/src 85% 0%
chain-extensions/dapps-staking/src 0% 0%
frame/custom-signatures/src 52% 0%
frame/dapps-staking/src 82% 0%
Summary 57% (2819 / 4916) 0% (0 / 0)

Minimum allowed line rate is 50%

@Dinonard
Copy link
Member Author

@shaunxw thanks for the approval!

I was thinking, instead of merging it directly, to keep this in a 'live' branch.
All consecutive PRs would be merged to this branch instead (or I could create another branch to aggregate all PRs).

This would keep the review of PRs light(er), while also providing one big overview in the end, before everything is merged.
What do you think?

@shaunxw
Copy link
Member

shaunxw commented Jun 14, 2023

Love this idea. It definitely helps with the review process.

@Dinonard Dinonard changed the base branch from polkadot-v0.9.39 to feat/dapp-staking-v3 June 14, 2023 08:24
@Dinonard Dinonard merged commit a3d4ac9 into feat/dapp-staking-v3 Jun 14, 2023
@Dinonard Dinonard deleted the feat/ds-v3-phase1 branch June 14, 2023 08:25
@Dinonard Dinonard mentioned this pull request Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants