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

Kusama People: Clear judgements #339

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

- Handle extra erroneous consumer reference when a nomination pool is destroying ([polkadot-fellows/runtimes#318](https://github.com/polkadot-fellows/runtimes/pull/318))
- Introduce [Encointer](https://encointer.org) collator selection and send fees to authors instead of treasury ([polkadot-fellows/runtimes#270](https://github.com/polkadot-fellows/runtimes/pull/270))
- Kusama People: clear requested judgements that do not have corresponding deposits reserved ([polkadot-fellows/runtimes#339](https://github.com/polkadot-fellows/runtimes/pull/339))

## [1.2.4] 20.05.2024

Expand Down
236 changes: 236 additions & 0 deletions system-parachains/people/people-kusama/src/identity_ops.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#[frame_support::pallet]
pub mod pallet_identity_ops {
use crate::*;
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use pallet_identity::Judgement;

/// Weight information for extrinsics in this pallet.
pub trait WeightInfo {
/// Weight for clearing judgement.
fn clear_judgement() -> Weight;
}

type IdentityPallet = pallet_identity::Pallet<Runtime>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using the concrete Runtime here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? Generic instead? This is valid only for the scope of this crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the pallet uses Pallet<T>, Error<T>, etc but still depends concretely on Runtime. Seems a bit inconsistent to me.

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 do not think it will compile without <T> argument for those types. For the rest where I can I use the actual type for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You think it worth generalizing these pallet for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that you are using it a number of other places as well - accessing the Balances pallet and such. So, probably not worth it at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pallet is meant to be removed by the following release. I do not see a practical reason to generalized it.


#[pallet::pallet]
pub struct Pallet<T>(PhantomData<T>);

#[pallet::config]
pub trait Config: frame_system::Config {
/// Overarching event type.
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
}

#[pallet::error]
pub enum Error<T> {
/// No judgement to clear.
NotFound,
}

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config> {
/// The invalid judgements have been cleared.
JudgementsCleared { target: AccountId },
}

pub(crate) type Identity = (
pallet_identity::Registration<
<Runtime as pallet_balances::Config>::Balance,
<Runtime as pallet_identity::Config>::MaxRegistrars,
<Runtime as pallet_identity::Config>::IdentityInformation,
>,
Option<BoundedVec<u8, <Runtime as pallet_identity::Config>::MaxUsernameLength>>,
);

/// Alias for `IdentityOf` from `pallet_identity`.
#[frame_support::storage_alias(pallet_name)]
pub(crate) type IdentityOf =
StorageMap<IdentityPallet, Twox64Concat, AccountId, Identity, OptionQuery>;

#[pallet::call]
impl<T: Config> Pallet<T> {
/// Clear requested judgements that do not have a corresponding deposit reserved.
///
/// This is successful only if the `target` account has judgements to clear. The transaction
/// fee is refunded to the caller if successful.
#[pallet::call_index(0)]
#[pallet::weight(weights::pallet_identity_ops::WeightInfo::<Runtime>::clear_judgement())]
pub fn clear_judgement(
_origin: OriginFor<T>,
target: AccountId,
) -> DispatchResultWithPostInfo {
let identity = IdentityPallet::identity(&target).ok_or(Error::<T>::NotFound)?;
let (removed, identity) = Self::do_clear_judgement(&target, identity);
ensure!(removed > 0, Error::<T>::NotFound);

IdentityOf::insert(&target, identity);
Copy link
Contributor

Choose a reason for hiding this comment

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

First thought was that this was inserting in the local pallet storage. Never seen the first generic in StorageMap be other than _ before (like here).

Maybe @ggwpez / @gupnik can check for my sanity that this will update the right storage value?

Copy link
Contributor

@gupnik gupnik Jun 4, 2024

Choose a reason for hiding this comment

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

Yeah, we use a similar pattern in migrations as well.


Self::deposit_event(Event::JudgementsCleared { target });

Ok(Pays::No.into())
}
}

impl<T: Config> Pallet<T> {
fn do_clear_judgement(account_id: &AccountId, mut identity: Identity) -> (u32, Identity) {
// `subs_of`'s query kind is value option, if non subs the deposit is zero.
let (subs_deposit, _) = IdentityPallet::subs_of(account_id);
// deposit without deposits for judgement request.
let identity_deposit = identity.0.deposit.saturating_add(subs_deposit);
// total reserved balance.
let reserved = Balances::reserved_balance(account_id);
// expected deposit with judgement deposits.
let mut expected_total_deposit = identity_deposit;
// count before cleaning up the judgements.
let judgements_count = identity.0.judgements.len();

identity.0.judgements.retain(|(_, judgement)| {
if let Judgement::FeePaid(deposit) = judgement {
expected_total_deposit = expected_total_deposit.saturating_add(*deposit);
reserved >= expected_total_deposit
} else {
true
}
});

((judgements_count - identity.0.judgements.len()) as u32, identity)
}
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
#[cfg(feature = "try-runtime")]
fn try_state(_: BlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can run this with

> cargo install --git https://github.com/paritytech/try-runtime-cli --locked  
> cargo build -p people-kusama-runtime --features try-runtime 
> try-runtime \                                                                                                                                    130 ↵
--runtime ./target/debug/wbuild/people-kusama-runtime/people_kusama_runtime.wasm \
execute-block \
--try-state IdentityOps \
live \
--uri wss://kusama-people-rpc.polkadot.io

use crate::Balance;
use sp_core::crypto::Ss58Codec;
use sp_runtime::traits::Zero;

let mut invalid_identity_count = 0;
let mut invalid_judgement_count = 0;
let mut identity_count = 0;
IdentityOf::iter().for_each(|(account_id, _)| {
let (identity, username) = IdentityPallet::identity(&account_id).unwrap();

let (paid_judgement_count, judgement_deposit) = identity.judgements.iter().fold(
(0, Zero::zero()),
|(count, total_deposit): (u32, Balance), (_, judgement)| {
if let Judgement::FeePaid(deposit) = judgement {
(count + 1, total_deposit.saturating_add(*deposit))
} else {
(count, total_deposit)
}
},
);

let (subs_deposit, _) = IdentityPallet::subs_of(&account_id);

let deposit =
identity.deposit.saturating_add(judgement_deposit).saturating_add(subs_deposit);
let deposit_wo_judgement = identity.deposit.saturating_add(subs_deposit);
let reserved = Balances::reserved_balance(&account_id);

if deposit > reserved && paid_judgement_count > 0 {
invalid_identity_count += 1;
invalid_judgement_count += paid_judgement_count;

log::info!(
"account with invalid state: {:?}, expected reserve at least: {:?}, actual: {:?}",
account_id.clone().to_ss58check(),
deposit,
reserved,
);

assert_eq!(
paid_judgement_count,
Self::do_clear_judgement(&account_id, (identity, username)).0
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with this we tests all existing cases where there is some judgement == Judgement::FeePaid(..) and deposit > reserved


if deposit_wo_judgement != reserved {
log::warn!(
"unexpected state: {:?}, deposit w/o judgement: {:?}, not equal to the total reserved: {:?}",
account_id.clone().to_ss58check(),
deposit_wo_judgement,
reserved,
);
}
} else {
assert_eq!(0, Self::do_clear_judgement(&account_id, (identity, username)).0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with this we tests all other cases result no change to judgements

}
if deposit_wo_judgement > reserved {
log::warn!(
"unexpected state: {:?}, deposit w/o judgement: {:?}, greater than the total reserved: {:?}",
account_id.clone().to_ss58check(),
deposit_wo_judgement,
reserved,
);
}
identity_count += 1;
});

log::info!("total identities processed: {:?}", identity_count);
log::info!("invalid identities: {:?}", invalid_identity_count);
log::info!("invalid judgements: {:?}", invalid_judgement_count);

Ok(())
}
}
}

#[cfg(feature = "runtime-benchmarks")]
#[frame_benchmarking::v2::benchmarks(where T: pallet_identity_ops::Config)]
mod benchmarks {
use crate::{people::IdentityInfo, *};
use frame_benchmarking::BenchmarkError;
use frame_system::RawOrigin;
use pallet_identity::{IdentityInformationProvider, Judgement};
use pallet_identity_ops::{Event, Identity, *};
use parachains_common::{AccountId, Balance};
use sp_core::Get;
use sp_runtime::traits::One;

#[benchmark]
fn clear_judgement() -> Result<(), BenchmarkError> {
let max_registrars =
<<Runtime as pallet_identity::Config>::MaxRegistrars as Get<u32>>::get();
let mut judgements = Vec::<(u32, Judgement<Balance>)>::new();
for i in 0..max_registrars {
judgements.push((i, Judgement::FeePaid(Balance::one())));
}
let identity: Identity = (
pallet_identity::Registration {
deposit: Balance::one(),
judgements: judgements.try_into().unwrap(),
info: IdentityInfo::create_identity_info(),
},
None,
);

let target: AccountId = [1u8; 32].into();

IdentityOf::insert(&target, identity);

#[extrinsic_call]
_(RawOrigin::None, target.clone());

crate::System::assert_last_event(Event::<Runtime>::JudgementsCleared { target }.into());

Ok(())
}
}
10 changes: 10 additions & 0 deletions system-parachains/people/people-kusama/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#[cfg(feature = "std")]
include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));

mod identity_ops;
pub mod people;
mod weights;
pub mod xcm_config;
Expand All @@ -41,6 +42,7 @@ use frame_system::{
limits::{BlockLength, BlockWeights},
EnsureRoot,
};
use identity_ops::pallet_identity_ops;
use pallet_xcm::{EnsureXcm, IsVoiceOfBody};
use parachains_common::{
message_queue::{NarrowOriginToSibling, ParaIdToSibling},
Expand Down Expand Up @@ -511,6 +513,10 @@ impl identity_migrator::Config for Runtime {
type WeightInfo = weights::polkadot_runtime_common_identity_migrator::WeightInfo<Runtime>;
}

impl pallet_identity_ops::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
}

// Create the runtime by composing the FRAME pallets that were previously configured.
construct_runtime!(
pub enum Runtime
Expand Down Expand Up @@ -548,6 +554,9 @@ construct_runtime!(

// To migrate deposits
IdentityMigrator: identity_migrator = 248,

// Identity operations pallet.
IdentityOps: pallet_identity_ops = 247,
}
);

Expand All @@ -572,6 +581,7 @@ mod benches {
[pallet_xcm, PalletXcmExtrinsiscsBenchmark::<Runtime>]
[pallet_xcm_benchmarks::fungible, XcmBalances]
[pallet_xcm_benchmarks::generic, XcmGeneric]
[pallet_identity_ops, IdentityOps]
);
}

Expand Down
1 change: 1 addition & 0 deletions system-parachains/people/people-kusama/src/weights/mod.rs

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

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

Loading