Dedup nomination targets #6307
Dedup nomination targets #6307
Conversation
There was a problem hiding this comment.
Just one comment; otherwise, lgtm!
We don’t need to migrate the single account. When they update their nomination, any duplicates will be removed automatically. (You could add a unit test to demonstrate that updating nominations removes old duplicates as well.)
Thank you for your PR!
aurexav
force-pushed
the
dedup-nominators
branch
from
6131b21 to
134f7f6
Compare
| @@ -1296,21 +1296,26 @@ pub mod pallet { | |||
| Error::<T>::TooManyTargets | |||
| ); | |||
|
|
|||
| let mut targets = targets | |||
| .into_iter() | |||
| .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) | |||
There was a problem hiding this comment.
Could we use AccountId here? I'm not sure why we are using the Lookup type here.
|
Hi, are we going to merge this? |
There was a problem hiding this comment.
nominate weight is proportional to N, with this PR it is actually O(n*log(n)) because we are sorting.
(rust is using timsort so worst case is O(n log n).)
Maybe the benchmark should be changed to actually benchmark for the worst case, but I think it is fine as it is.
Since n is a small number (16 for dot, 24 for kusama), I think this is okay. |
|
@kianenigma Could you review the changes to see if they’re acceptable to you and merge them? They LGTM. |
Deduplicate the targets during the nomination process.
And I'm not pretty sure this is designed to be liked this.
This was found by the Subscan team. There is only one such special account in Polkadot(need a migration?). The Polkadot Apps UI will filter the selected targets for the users. I assume this uses a custom client to submit the extrinsic.
Note: the targets length affect the weight.
Or should we just return an error?
Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y