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

Improve documentation for fast-unstake pallet #14101

Merged
merged 41 commits into from
May 29, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
dbf2572
improve documentation of fast-unstake pallet
kianenigma May 7, 2023
9d2a9a2
using Sam's crate now
kianenigma May 9, 2023
bb11dc0
merge
kianenigma May 9, 2023
91e811b
fix
kianenigma May 9, 2023
85f4390
remove stuff not needed
kianenigma May 9, 2023
1a6c63d
Some updates
kianenigma May 9, 2023
dc82e48
use new prelude.
kianenigma May 9, 2023
4973a32
update
kianenigma May 10, 2023
8d0c523
some other fancy docs
kianenigma May 10, 2023
1572dd8
Update frame/fast-unstake/src/lib.rs
kianenigma May 10, 2023
9aa4ede
Update frame/support/procedural/src/pallet/expand/mod.rs
kianenigma May 10, 2023
f24ac3c
update
kianenigma May 10, 2023
048e061
Merge branch 'kiz-improve-fast-unstake-dcos' of github.com:paritytech…
kianenigma May 10, 2023
91846b9
Update frame/fast-unstake/src/lib.rs
kianenigma May 10, 2023
99cc532
Master.into()
kianenigma May 23, 2023
6b7804a
update
kianenigma May 23, 2023
c6a9dc7
fix no_std issue by updating to latest version of docify
sam0x17 May 25, 2023
6fda397
get things compiling by adding a use for StakingInterface
sam0x17 May 25, 2023
372f14e
fix docify paths to their proper values, still not working because bug
sam0x17 May 25, 2023
f68b1bb
embed working :tada:
sam0x17 May 25, 2023
3b75895
Merge remote-tracking branch 'origin/master' into kiz-improve-fast-un…
sam0x17 May 26, 2023
99ab314
update syn
sam0x17 May 26, 2023
acd122b
upgrade to docify v0.1.10 for some fixes
sam0x17 May 26, 2023
44e34e4
Apply suggestions from code review
kianenigma May 26, 2023
d9c814d
Merge branch 'master' of github.com:paritytech/substrate into kiz-imp…
kianenigma May 26, 2023
7cb1db4
improve docs
kianenigma May 26, 2023
5244952
Update frame/support/procedural/src/pallet/expand/doc_only.rs
kianenigma May 26, 2023
9c9ab28
fmt
kianenigma May 26, 2023
8976e53
Merge branch 'kiz-improve-fast-unstake-dcos' of github.com:paritytech…
kianenigma May 26, 2023
396cec2
fix
kianenigma May 26, 2023
21fc4df
Update frame/support/procedural/src/pallet/expand/doc_only.rs
kianenigma May 28, 2023
e4939dc
Update frame/support/procedural/src/pallet/expand/config.rs
kianenigma May 28, 2023
ffe579f
Update frame/support/procedural/src/pallet/expand/config.rs
kianenigma May 28, 2023
04a5aff
remove redundant
kianenigma May 28, 2023
9d7fb36
Merge branch 'kiz-improve-fast-unstake-dcos' of github.com:paritytech…
kianenigma May 28, 2023
ff78017
update docify rev
kianenigma May 29, 2023
351ee05
Merge remote-tracking branch 'origin/master' into kiz-improve-fast-un…
May 29, 2023
9602902
update.
kianenigma May 29, 2023
93aca7a
update.
kianenigma May 29, 2023
55fc001
update lock file
kianenigma May 29, 2023
851e4af
Merge branch 'master' of github.com:paritytech/substrate into kiz-imp…
kianenigma May 29, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions frame/fast-unstake/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ frame-election-provider-support = { default-features = false, path = "../electio

frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, path = "../benchmarking" }

docify = "0.1.1"

[dev-dependencies]
pallet-staking-reward-curve = { version = "4.0.0-dev", path = "../staking/reward-curve" }
sp-core = { version = "7.0.0", default-features = false, path = "../../primitives/core" }
Expand Down
189 changes: 141 additions & 48 deletions frame/fast-unstake/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,101 @@
// See the License for the specific language governing permissions and
// limitations under the License.

//! A pallet that's designed to JUST do the following:
//! > Made with *Substrate*, for *Polkadot*.
//!
//! If a nominator is not exposed in any `ErasStakers` (i.e. "has not actively backed any
//! validators in the last `BondingDuration` days"), then they can register themselves in this
//! [![github]](https://github.com/paritytech/substrate/frame/fast-unstake) -
//! [![polkadot]](https://polkadot.network)
//!
//! [polkadot]: https://img.shields.io/badge/polkadot-E6007A?style=for-the-badge&logo=polkadot&logoColor=white
//! [github]: https://img.shields.io/badge/github-8da0cb?style=for-the-badge&labelColor=555555&logo=github
//!
//! # Fast Unstake Pallet
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//!
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! A pallet to allow participants of the staking system (represented by [`Config::Staking`], being
//! [`StakingInterface`]) to unstake quicker, if and only if they meed the condition of not being
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! exposed to any slashes.
//!
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! ## High Level / End-User Details
//!
//! This pallet that's designed to JUST do the following:
Copy link

Choose a reason for hiding this comment

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

Suggested change
//! This pallet that's designed to JUST do the following:
//! This pallet is designed to JUST do the following:

Although TBH I'm not sure what purpose this line serves, can it be removed?

Also, since we are improving the docs: I feel like the text that follows could use some structure, e.g. subsections or bullet points. Right now it seems (to me) like an unapproachable wall of text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find your comment about this sentence fair, but not the rest. It is organized into a series of paragraphs and is following a logical chain. Perhaps if you describe further which part of it is unapprochable, I can improve.

Nonetheless though, this is in realm of subjective measures harder to align on. I don't think we can ever make everyone write docs that are understandable to everyone. Simply things like different ways of thinking and language levels will make that impossible.

What we want to ensure is at least following the structure mentioned in #14115, and doing your best.

Copy link

Choose a reason for hiding this comment

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

Fair enough, it was indeed a subjective impression I got as someone new to this pallet. Reading my post again it sounds more critical than I intended it to be, sorry. 🙈 To be concrete, I would suggest adding subsections like e.g. "Registration" and "Checking". Feel free to ignore if you think that's unnecessary. The new docs are already awesome as is!

//!
//! If a nominator is not exposed anywhere in the staking system, checked via
//! [`StakingInterface::is_exposed_in_era`] (i.e. "has not actively backed any validators in the
//! last [`StakingInterface::bonding_duration`] days"), then they can register themselves in this
//! pallet, unstake faster than having to wait an entire bonding duration.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//!
//! Appearing in the exposure of a validator means being exposed equal to that validator from the
//! point of view of the staking system. This usually means earning rewards with the validator, and
//! also being at the risk of slashing with the validator. This is equivalent to the "Active
//! Nominator" role explained in the
//! [February Staking Update](https://polkadot.network/blog/staking-update-february-2022/).
//! *Being exposed with validator* from the point of view of the staking system means earning
//! rewards with the validator, and also being at the risk of slashing with the validator. This is
//! equivalent to the "Active Nominator" role explained in
//! [here](https://polkadot.network/blog/staking-update-february-2022/).
//!
//! Stakers who are certain about NOT being exposed can register themselves with
//! [`Pallet::register_fast_unstake`]. This will chill, and fully unbond the staker, and place them
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! in the queue to be checked.
//!
//! A successful registration implies being fully unbonded and chilled in the staking system. These
//! effects persist, even if the fast-unstake registration is retracted (see [`Pallet::deregister`]
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! and further).
//!
//! Once registered as a fast-unstaker, the staker will be queued and checked by the system. This
//! can take a variable number of blocks based on demand, but will almost certainly be "faster" (as
//! the name suggest) than waiting the standard bonding duration.
//!
//! A fast-unstaker is either in [`Queue`], or actively being checked, at which point it lives in
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! [`Head`]. Once in [`Head`], the request cannot retracted anymore. But, once in [`Queue`], it
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! can, via [`Pallet::deregister`].
//!
//! A deposit, equal to [`Config::Deposit`] is collected for this process, and is returned in case a
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//! successful unstake occurs (`Event::Unstaked` signals that).
//!
//! Once processed, if successful, no additional fee for the checking process is taken, and the
//! staker is instantly unbonded.
//!
//! If unsuccessful, meaning that the staker was exposed, the aforementioned deposit will be slashed
//! for the amount of wasted work they have inflicted on the chian.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//!
//! All in all, this pallet is meant to provide an easy off-ramp for some stakers.
//!
//! ### Example
//!
//! 1. Fast-unstake with multiple participants in the queue.
#![doc = docify::embed!("./frame/fast-unstake/src/tests.rs", successful_multi_queue)]
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
//!
//! 2. Fast unstake failing because a nominator is exposed.
#![doc = docify::embed!("./frame/fast-unstake/src/tests.rs", exposed_nominator_cannot_unstake)]
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
//!
//! ## Code Details
//!
//! See [`pallet`] module for more information.
//!
//! ## Low Level / Implementation Details
//!
//! This pallet works off the basis of `on_idle`, meaning that it provides no guarantee about when
//! it will succeed, if at all. Moreover, the queue implementation is unordered. In case of
//! congestion, no FIFO ordering is provided.
//!
//! Stakers who are certain about NOT being exposed can register themselves with
//! [`Call::register_fast_unstake`]. This will chill, and fully unbond the staker, and place them in
//! the queue to be checked.
//! A few important considerations can be concluded based on the `on_idle`-based implementation:
//!
//! Once queued, but not being actively processed, stakers can withdraw their request via
//! [`Call::deregister`].
//! * It is crucial for the weights of this pallet to be correct. The code inside
//! [`Pallet::on_idle`] MUST be able to measure itself and report the remaining weight correctly
//! after execution.
//!
//! Once queued, a staker wishing to unbond can perform no further action in pallet-staking. This is
//! to prevent them from accidentally exposing themselves behind a validator etc.
//! * If the weight measurement is incorrect, it can lead to perpetual overweight (consequently
//! slow) blocks.
//!
//! Once processed, if successful, no additional fee for the checking process is taken, and the
//! staker is instantly unbonded.
//! * The amount of weight that `on_idle` consumes is a direct function of [`ErasToCheckPerBlock`].
//!
//! If unsuccessful, meaning that the staker was exposed sometime in the last `BondingDuration` eras
//! they will end up being slashed for the amount of wasted work they have inflicted on the chian.
//! * Thus, a correct value of [`ErasToCheckPerBlock`] (which can be set via [`Pallet::control`])
//! should be chosen, such that a reasonable amount of weight is used `on_idle`. If
//! [`ErasToCheckPerBlock`] is too large, `on_idle` will always conclude that it has not enough
//! weight to proceed, and will early-return. Nonetheless, this should also be *safe* as long as
//! the benchmarking/weights are *accurate*.
//!
//! * See the inline code-comments on `do_on_idle` (private) for more details.
//!
//! * For further safety, in case of any unforeseen errors, the pallet will emit
//! [`Event::InternalError`] and set [`ErasToCheckPerBlock`] back to 0, which essentially means
//! the pallet will halt/disable itself.

#![cfg_attr(not(feature = "std"), no_std)]

Expand All @@ -64,9 +128,15 @@ pub mod migrations;
pub mod types;
pub mod weights;

// some extra imports for docs to link properly.
#[cfg(doc)]
pub use frame_support::traits::Hooks;
#[cfg(doc)]
pub use sp_staking::StakingInterface;

/// The logging target of this pallet.
pub const LOG_TARGET: &'static str = "runtime::fast-unstake";

// syntactic sugar for logging.
#[macro_export]
macro_rules! log {
($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
Expand All @@ -91,16 +161,6 @@ pub mod pallet {
use sp_std::{prelude::*, vec::Vec};
pub use weights::WeightInfo;

#[derive(scale_info::TypeInfo, codec::Encode, codec::Decode, codec::MaxEncodedLen)]
#[codec(mel_bound(T: Config))]
#[scale_info(skip_type_params(T))]
pub struct MaxChecking<T: Config>(sp_std::marker::PhantomData<T>);
impl<T: Config> frame_support::traits::Get<u32> for MaxChecking<T> {
fn get() -> u32 {
T::Staking::bonding_duration() + 1
}
}

const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);

#[pallet::pallet]
Expand All @@ -122,7 +182,7 @@ pub mod pallet {
#[pallet::constant]
type Deposit: Get<BalanceOf<Self>>;

/// The origin that can control this pallet.
/// The origin that can control this pallet, in other words invoke [`Pallet::control`].
type ControlOrigin: frame_support::traits::EnsureOrigin<Self::RuntimeOrigin>;

/// Batch size.
Expand All @@ -133,56 +193,60 @@ pub mod pallet {
/// The access to staking functionality.
type Staking: StakingInterface<Balance = BalanceOf<Self>, AccountId = Self::AccountId>;

/// Maximum value for `ErasToCheckPerBlock`, checked in [`Pallet::control`].
///
/// This should be as close as possible, but more than the actual value, in order to have
/// accurate benchmarks.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
type MaxErasToCheckPerBlock: Get<u32>;

/// The weight information of this pallet.
type WeightInfo: WeightInfo;

/// Maximum value for `ErasToCheckPerBlock`. This should be as close as possible, but more
/// than the actual value, in order to have accurate benchmarks.
type MaxErasToCheckPerBlock: Get<u32>;

/// Use only for benchmarking.
#[cfg(feature = "runtime-benchmarks")]
type MaxBackersPerValidator: Get<u32>;
}

/// The current "head of the queue" being unstaked.
///
/// The head in itself can be a batch of up to [`Config::BatchSize`] stakers.
#[pallet::storage]
pub type Head<T: Config> = StorageValue<_, UnstakeRequest<T>, OptionQuery>;

/// The map of all accounts wishing to be unstaked.
///
/// Keeps track of `AccountId` wishing to unstake and it's corresponding deposit.
///
/// TWOX-NOTE: SAFE since `AccountId` is a secure hash.
// Hasher: Twox safe since `AccountId` is a secure hash.
#[pallet::storage]
pub type Queue<T: Config> = CountedStorageMap<_, Twox64Concat, T::AccountId, BalanceOf<T>>;

/// Number of eras to check per block.
///
/// If set to 0, this pallet does absolutely nothing.
/// If set to 0, this pallet does absolutely nothing. Cannot be set to more than
/// [`Config::MaxErasToCheckPerBlock`].
///
/// Based on the amount of weight available at `on_idle`, up to this many eras of a single
/// nominator might be checked.
/// Based on the amount of weight available at [`Pallet::on_idle`], up to this many eras of a
/// [`UnstakeRequest`], stored in [`Head`] might be checked.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this a bit confusing, can it be rephrased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, my english sometimes tends to be way too convoluted :)) hopefully better now.

#[pallet::storage]
#[pallet::getter(fn eras_to_check_per_block)]
pub type ErasToCheckPerBlock<T: Config> = StorageValue<_, u32, ValueQuery>;

/// The events of this pallet.
#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config> {
/// A staker was unstaked.
Unstaked { stash: T::AccountId, result: DispatchResult },
/// A staker was slashed for requesting fast-unstake whilst being exposed.
Slashed { stash: T::AccountId, amount: BalanceOf<T> },
/// An internal error happened. Operations will be paused now.
InternalError,
/// A batch was partially checked for the given eras, but the process did not finish.
BatchChecked { eras: Vec<EraIndex> },
/// A batch of a given size was terminated.
///
/// This is always follows by a number of `Unstaked` or `Slashed` events, marking the end
/// of the batch. A new batch will be created upon next block.
BatchFinished { size: u32 },
/// An internal error happened. Operations will be paused now.
InternalError,
}

#[pallet::error]
Expand Down Expand Up @@ -244,8 +308,12 @@ pub mod pallet {
impl<T: Config> Pallet<T> {
/// Register oneself for fast-unstake.
///
/// The dispatch origin of this call must be signed by the controller account, similar to
/// `staking::unbond`.
/// ## Dispatch Origin
///
/// The dispatch origin of this call must be *signed* by the whoever is permitted by to call
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// unbond funds by the staking system. See [`Config::Staking`].
///
/// ## Details
///
/// The stash associated with the origin must have no ongoing unlocking chunks. If
/// successful, this will fully unbond and chill the stash. Then, it will enqueue the stash
Expand All @@ -260,6 +328,10 @@ pub mod pallet {
/// If the check fails, the stash remains chilled and waiting for being unbonded as in with
/// the normal staking system, but they lose part of their unbonding chunks due to consuming
/// the chain's resources.
///
/// ## Events
///
/// Some events from the staking and currency system might be emitted.
#[pallet::call_index(0)]
#[pallet::weight(<T as Config>::WeightInfo::register_fast_unstake())]
pub fn register_fast_unstake(origin: OriginFor<T>) -> DispatchResult {
Expand All @@ -285,11 +357,22 @@ pub mod pallet {

/// Deregister oneself from the fast-unstake.
///
/// ## Dispatch Origin
///
/// The dispatch origin of this call must be *signed* by the whoever is permitted by to call
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// unbond funds by the staking system. See [`Config::Staking`].
///
/// ## Details
///
/// This is useful if one is registered, they are still waiting, and they change their mind.
///
/// Note that the associated stash is still fully unbonded and chilled as a consequence of
/// calling `register_fast_unstake`. This should probably be followed by a call to
/// `Staking::rebond`.
/// calling [`Pallet::register_fast_unstake`]. THerefore, this should probably be followed
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// by a call to `rebond` in the staking system.
///
/// ## Events
///
/// Some events from the staking and currency system might be emitted.
#[pallet::call_index(1)]
#[pallet::weight(<T as Config>::WeightInfo::deregister())]
pub fn deregister(origin: OriginFor<T>) -> DispatchResult {
Expand All @@ -315,7 +398,17 @@ pub mod pallet {

/// Control the operation of this pallet.
///
/// Dispatch origin must be signed by the [`Config::ControlOrigin`].
/// ## Dispatch Origin
///
/// The dispatch origin of this call must be [`Config::ControlOrigin`].
///
/// ## Details
///
/// Can set the number of eras to check per block, and potentially other admin work.
///
/// ## Events
///
/// No events are emitted from this dispatch.
#[pallet::call_index(2)]
#[pallet::weight(<T as Config>::WeightInfo::control())]
pub fn control(origin: OriginFor<T>, eras_to_check: EraIndex) -> DispatchResult {
Expand Down
Loading