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

dapp staking v3 - Freeze Improvements #1164

Conversation

Dinonard
Copy link
Member

@Dinonard Dinonard commented Feb 6, 2024

Overview

dApp staking v3 will now check for total balance when locking (freezing) some amount, instead of just free balance.
This is due to the freeze overlapping with the reserve/holds.

This PR addresses the collator selection issue, where slash can happen due to missed blocks.
There's still the remaining issue of pallet-identity but the call that can cause slash is privileged so for now we can consider it to be ok.

Pallets

  • updates collator selection pallet to check whether an candidate account is participating in dApp staking
    • doesn't apply to invulnerables
  • updates dApp staking migration by removing some checks which are no longer applicable
  • updates dApp staking v3 to check whether an account trying to enter dapp Staking is a candidate in collator selection

Runtime

  • integrates new changes into all applicable runtimes
  • added new integration tests

For UI Team

There are essentially two changes to account for:

1. Available Lock Balance Change

// Calculate & check amount available for locking
let available_balance = T::Currency::total_balance(&account).saturating_sub(ledger.active_locked_amount());

total_balance(...) of an account is the sum of free and reserved balances:

pub struct AccountData<Balance> {
	/// Non-reserved part of the balance which the account holder may be able to control.
	///
	/// This is the only balance that matters in terms of most operations on tokens.
	pub free: Balance,
	/// Balance which is has active holds on it and may not be used at all.
	///
	/// This is the sum of all individual holds together with any sums still under the (deprecated)
	/// reserves API.
	pub reserved: Balance,
	/// The amount that `free + reserved` may not drop below when reducing the balance, except for
	/// actions where the account owner cannot reasonably benefit from the balance reduction, such
	/// as slashing.
	pub frozen: Balance,
	/// Extra information about this account. The MSB is a flag indicating whether the new ref-
	/// counting logic is in place for this account.
	pub flags: ExtraFlags,
}

...

	pub fn total(&self) -> Balance {
		self.free.saturating_add(self.reserved)
	}

2. Collator Not Allowed To Participate in dApp Staking

If the database entry CollatorSelection::Candidates contains the staker account, lock operation will fail with AccountNotAvailableForDappStaking error.
Screenshot 2024-02-06 at 11 17 08

Check list

  • Solution implemented
  • Tested
  • Re-run benchmarks

@Dinonard Dinonard changed the base branch from master to feat/shiden-ds-v3-t2-integration February 6, 2024 09:01
@Dinonard Dinonard self-assigned this Feb 6, 2024
@Dinonard Dinonard added shiden related to shiden runtime shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Feb 6, 2024
@Dinonard Dinonard marked this pull request as ready for review February 6, 2024 10:18
@Dinonard
Copy link
Member Author

Dinonard commented Feb 6, 2024

/bench shiden-dev pallet_dapp_staking_v3,pallet_collator_selection

Copy link

github-actions bot commented Feb 6, 2024

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7798144646.
Please wait for a while.
Branch: feat/dapp-staking-v3-freeze-improvements
SHA: b679ecd

Copy link

github-actions bot commented Feb 6, 2024

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7798144646.

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.

LGTM. Maybe the two AccountCheck trait definitions could be merged into one?

@Dinonard
Copy link
Member Author

Dinonard commented Feb 6, 2024

LGTM. Maybe the two AccountCheck trait definitions could be merged into one?

Initially I used frame_support::traits::Contains but figured I'd prefer to have a strictly distinct types to accidentally avoid mixing them up. Perhaps it's an overkill?

Copy link

github-actions bot commented Feb 6, 2024

Code Coverage

Package Line Rate Branch Rate Health
chain-extensions/types/assets/src 0% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/dynamic-evm-base-fee/src 92% 0%
pallets/xvm/src 51% 0%
pallets/unified-accounts/src 84% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/dapps-staking/src 94% 0%
pallets/astar-xcm-benchmarks/src 89% 0%
pallets/inflation/src 80% 0%
precompiles/unified-accounts/src 100% 0%
pallets/block-rewards-hybrid/src 91% 0%
pallets/dapp-staking-migration/src 49% 0%
precompiles/assets-erc20/src 81% 0%
chain-extensions/types/xvm/src 0% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
pallets/dapp-staking-v3/src 88% 0%
chain-extensions/xvm/src 0% 0%
precompiles/substrate-ecdsa/src 74% 0%
primitives/src/xcm 66% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/collator-selection/src 89% 0%
pallets/dapps-staking/src 90% 0%
pallets/ethereum-checked/src 75% 0%
precompiles/xcm/src 72% 0%
primitives/src 59% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
chain-extensions/unified-accounts/src 0% 0%
pallets/dapps-staking/src/pallet 86% 0%
chain-extensions/types/unified-accounts/src 0% 0%
chain-extensions/pallet-assets/src 56% 0%
precompiles/xvm/src 74% 0%
pallets/xc-asset-config/src 64% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
precompiles/sr25519/src 64% 0%
pallets/static-price-provider/src 58% 0%
Summary 79% (4389 / 5564) 0% (0 / 0)

Minimum allowed line rate is 50%

@Dinonard Dinonard merged commit 9865dcb into feat/shiden-ds-v3-t2-integration Feb 6, 2024
8 checks passed
@Dinonard Dinonard deleted the feat/dapp-staking-v3-freeze-improvements branch February 6, 2024 12:53
Dinonard added a commit that referenced this pull request Feb 6, 2024
* dApp staking v3 & Tokenomics 2.0 - Shiden Integration

* Full integration

* Changes

* Fix

* Proper fix, really

* Fix for ed issue

* Fix formatting

* Weights

* Resolve TODOs

* Minor changes

* Fmt fix

* Bump versions

* Comment

* dapp staking v3 - Freeze Improvements (#1164)

* Init commit

* Collator selection change

* Collator selection update

* Integration tests

* Update astar spec due to changes

* Weight updates
@shaunxw
Copy link
Member

shaunxw commented Feb 7, 2024

Initially I used frame_support::traits::Contains but figured I'd prefer to have a strictly distinct types to accidentally avoid mixing them up. Perhaps it's an overkill?

It's all right. I don't have a strong opinion, just a small suggestion. Two distinct types help with readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya shiden related to shiden runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants