Conversation
bkchr
added
A0-please_review
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
| Proposals::<T>::iter().try_for_each(|(_, prop)| | ||
| if validators.iter().all(|v| !prop.validators.contains(v)) { | ||
| Ok(()) | ||
| } else { | ||
| Err(Error::<T>::ValidatorAlreadyRegistered) | ||
| } | ||
| )?; |
There was a problem hiding this comment.
This is pretty aggressive, if I were writing a production pallet, i would try to catch this error on approval. If i approve a pallet, an the validator is already registered in the system, then it would fail. This should prevent any duplicate registrations from getting into the pallet.
There was a problem hiding this comment.
Versus looking through all proposals proactively, which really has no bounds as far as I can tell.
There was a problem hiding this comment.
I see what you mean. However, even on approval we should still iterate over all approved proposals to search for validators.
I hope that this pallet never gets used for anything than for this test network, so I think it is okay to be "aggressive" 🙈
| Either::Right(who) => Some(who), | ||
| }; | ||
|
|
||
| Self::is_approved_or_scheduled(para_id)?; |
There was a problem hiding this comment.
This seems like it should be the last check among these checks since there is an iterator inside
shawntabrizi
left a comment
shawntabrizi
left a comment
There was a problem hiding this comment.
I didn't look at this like a production pallet. For test pallet, at high level, seems fine.
* Adds propose parachain pallet * Update runtime/rococo/src/propose_parachain.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Fix runtime benchmarks * Get rid of staking * Fix benchmarking feature.. * Remove accidentally added crate * Bump Rococo spec_version Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
3 participants
No description provided.