Kusama People: Clear judgements #339

muharem · 2024-06-02T16:49:05Z

Clear requested judgements that do not have corresponding deposits reserved.

These judgements exist because the data migration did not take into consideration the deposits for requested judgements.

More info: #315 (comment)

muharem · 2024-06-03T09:06:27Z

system-parachains/people/people-kusama/src/identity_ops.rs

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


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

muharem · 2024-06-03T09:07:50Z

system-parachains/people/people-kusama/src/identity_ops.rs

+					assert_eq!(
+						paid_judgement_count,
+						Self::do_clear_judgement(&account_id, (identity, username)).0
+					);


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

muharem · 2024-06-03T09:08:28Z

system-parachains/people/people-kusama/src/identity_ops.rs

+						);
+					}
+				} else {
+					assert_eq!(0, Self::do_clear_judgement(&account_id, (identity, username)).0);


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

joepetrowski · 2024-06-03T18:39:12Z

system-parachains/people/people-kusama/src/identity_ops.rs

+			let (removed, identity) = Self::do_clear_judgement(&target, identity);
+			ensure!(removed > 0, Error::<T>::NotFound);
+
+			IdentityOf::insert(&target, identity);


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?

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

bkchr

Why is this not just a migration? We are talking about 82 accounts?

bkchr · 2024-06-03T20:19:43Z

system-parachains/people/people-kusama/src/identity_ops.rs

+		/// Weight calculation for the worst-case scenario of `clear_judgement`.
+		/// Equal to 1 identity read/write + 1 subs read + 1 reserve balance read.
+		fn weight_clear_judgement() -> Weight {
+			<Runtime as frame_system::Config>::DbWeight::get().reads_writes(3, 1)
+		}


We need a benchmark. This is a parachain and thus, we need to know the pov_size

Updated with the benchmark function and the weight from the target machine

muharem · 2024-06-04T10:11:59Z

Why is this not just a migration? We are talking about 82 accounts?

Listing 82 accounts in migration feels less friendly to me to verify (by reviewer) and to store in the runtime. Also would need to make sure it all will fit into a single block.

gupnik · 2024-06-04T10:40:48Z

system-parachains/people/people-kusama/src/identity_ops.rs

+	use frame_system::pallet_prelude::*;
+	use pallet_identity::Judgement;
+
+	type IdentityPallet = pallet_identity::Pallet<Runtime>;


Why are we using the concrete Runtime here?

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

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

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.

You think it worth generalizing these pallet for consistency?

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.

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

gupnik · 2024-06-04T10:48:15Z

system-parachains/people/people-kusama/src/identity_ops.rs

+			let (removed, identity) = Self::do_clear_judgement(&target, identity);
+			ensure!(removed > 0, Error::<T>::NotFound);
+
+			IdentityOf::insert(&target, identity);


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

muharem · 2024-06-04T12:44:41Z

Why is this not just a migration? We are talking about 82 accounts?

I have submitted the benchmark code and am waiting for access to generate the weights.

Other than what I've mentioned above regarding my reasoning for using a transaction call instead of a storage migration, my personal opinion is that storage migrations are primarily meant to map a pallet's data to a new storage version. When faced with an issue like this, I first consider if it can be solved with a regular transaction call.

If you believe this should be done with a migration, please let me know, and I will rewrite it accordingly.

system-parachains/people/people-kusama/src/weights/pallet_identity_ops.rs

Co-authored-by: Branislav Kontur <[email protected]>

bkchr · 2024-06-05T11:51:07Z

Other than what I've mentioned above regarding my reasoning for using a transaction call instead of a storage migration, my personal opinion is that storage migrations are primarily meant to map a pallet's data to a new storage version. When faced with an issue like this, I first consider if it can be solved with a regular transaction call.

With a transaction you will need a bot or similar that does the job. A migration is (IMO) a cleaner approach here. Especially for such a small runtime upgrade. But if you want to keep it as a transaction and will also ensure that all are migrated, I don't care :D

muharem · 2024-06-05T13:49:33Z

Other than what I've mentioned above regarding my reasoning for using a transaction call instead of a storage migration, my personal opinion is that storage migrations are primarily meant to map a pallet's data to a new storage version. When faced with an issue like this, I first consider if it can be solved with a regular transaction call.

With a transaction you will need a bot or similar that does the job. A migration is (IMO) a cleaner approach here. Especially for such a small runtime upgrade. But if you want to keep it as a transaction and will also ensure that all are migrated, I don't care :D

Yes, I would prefer to keep it the current way. It is also possible that the static account list that is correct at the moment of the release publication, wont be correct at the moment it will be executed (if you cancel_judgement and request again for example). I am also not sure how to provide a benchmark for such migration.

muharem · 2024-06-10T11:09:27Z

I have included for the removal the judgements with 0 deposits (Judgement::FeePaid(0)).
Now according to the clear logic in this PR, there are 89 candidates for the removal, the same identities as in the list from the comment, except one identity for the account DG6egJCfrcNPMSFYihxxPxtWvMwq3GR5k3pUS4pHouamuTv.
This account already received the judgement, and have total reserve balance as 0. Because all of it was transferred to the registrar. I think it is reasonable to keep it as it is for this account.

joepetrowski · 2024-06-10T11:13:06Z

This account already received the judgement, and have total reserve balance as 0. Because all of it was transferred to the registrar. I think it is reasonable to keep it as it is for this account.

How is this possible? Even if the registrar had a fee of zero, wouldn't the status change from FeePaid to the actual judgement?

muharem · 2024-06-10T11:16:21Z

This account already received the judgement, and have total reserve balance as 0. Because all of it was transferred to the registrar. I think it is reasonable to keep it as it is for this account.

How is this possible? Even if the registrar had a fee of zero, wouldn't the status change from FeePaid to the actual judgement?

Yes, it did receive the actual judgement from registrar, and transferred all it's reserved balance to registrar.

muharem · 2024-06-10T11:20:20Z

How is this possible? Even if the registrar had a fee of zero, wouldn't the status change from FeePaid to the actual judgement?

it was not zero. but when the judgement is provided, it uses Currency::repatriate_reserved to reward a registrar, and it transfers as much as possible (in our case all account's reserve balance).

github-actions · 2024-06-11T15:51:34Z

Review required! Latest push from author must always be reviewed

joepetrowski · 2024-06-13T07:40:44Z

/merge

fellowship-merge-bot · 2024-06-13T07:57:07Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

muharem added 3 commits June 2, 2024 18:47

clear judgement

82cbd76

fixes

1f32485

changelog

4d6808a

muharem marked this pull request as ready for review June 3, 2024 07:28

muharem mentioned this pull request Jun 3, 2024

Remove Identity from Kusama Relay #315

Merged

muharem commented Jun 3, 2024

View reviewed changes

muharem mentioned this pull request Jun 3, 2024

v1.2.5 Release #316

Closed

7 tasks

joepetrowski reviewed Jun 3, 2024

View reviewed changes

bkchr reviewed Jun 3, 2024

View reviewed changes

gupnik reviewed Jun 4, 2024

View reviewed changes

benchmarks

5c52a3a

local weights

a0a05c1

bkontur reviewed Jun 4, 2024

View reviewed changes

system-parachains/people/people-kusama/src/weights/pallet_identity_ops.rs Outdated Show resolved Hide resolved

muharem and others added 2 commits June 4, 2024 15:39

Apply suggestions from code review

af5149f

Co-authored-by: Branislav Kontur <[email protected]>

fix weights

a012e5c

muharem requested review from bkchr, bkontur, gupnik and joepetrowski June 4, 2024 13:45

joepetrowski mentioned this pull request Jun 6, 2024

v1.2.6 Release #344

Closed

4 tasks

remove fee paid with zero deposit

1a7d89b

muharem closed this Jun 10, 2024

muharem reopened this Jun 10, 2024

joepetrowski approved these changes Jun 11, 2024

View reviewed changes

joepetrowski and others added 3 commits June 11, 2024 13:13

Merge branch 'main' into muharem-clear-judgement

b52a7f8

Update CHANGELOG.md

2f031cf

Merge branch 'main' into muharem-clear-judgement

7dbafd9

github-actions bot requested a review from joepetrowski June 11, 2024 15:51

gupnik approved these changes Jun 12, 2024

View reviewed changes

joepetrowski approved these changes Jun 12, 2024

View reviewed changes

xlc approved these changes Jun 13, 2024

View reviewed changes

fellowship-merge-bot bot merged commit cede94a into polkadot-fellows:main Jun 13, 2024
37 of 40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kusama People: Clear judgements #339

Kusama People: Clear judgements #339

muharem commented Jun 2, 2024 •

edited

Loading

muharem Jun 3, 2024

muharem Jun 3, 2024

muharem Jun 3, 2024

joepetrowski Jun 3, 2024

gupnik Jun 4, 2024 •

edited

Loading

bkchr left a comment

bkchr Jun 3, 2024

muharem Jun 4, 2024

muharem commented Jun 4, 2024

gupnik Jun 4, 2024

muharem Jun 4, 2024

gupnik Jun 4, 2024

muharem Jun 4, 2024

muharem Jun 4, 2024

gupnik Jun 5, 2024

muharem Jun 5, 2024

gupnik Jun 4, 2024 •

edited

Loading

muharem commented Jun 4, 2024

bkchr commented Jun 5, 2024

muharem commented Jun 5, 2024

muharem commented Jun 10, 2024

joepetrowski commented Jun 10, 2024

muharem commented Jun 10, 2024

muharem commented Jun 10, 2024

github-actions bot commented Jun 11, 2024

joepetrowski commented Jun 13, 2024

fellowship-merge-bot bot commented Jun 13, 2024

Kusama People: Clear judgements #339

Kusama People: Clear judgements #339

Conversation

muharem commented Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gupnik Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gupnik Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

muharem commented Jun 4, 2024

bkchr commented Jun 5, 2024

muharem commented Jun 5, 2024

muharem commented Jun 10, 2024

joepetrowski commented Jun 10, 2024

muharem commented Jun 10, 2024

muharem commented Jun 10, 2024

github-actions bot commented Jun 11, 2024

joepetrowski commented Jun 13, 2024

fellowship-merge-bot bot commented Jun 13, 2024

muharem commented Jun 2, 2024 •

edited

Loading

gupnik Jun 4, 2024 •

edited

Loading

gupnik Jun 4, 2024 •

edited

Loading