fix: disable UD when membership removed
- Désactiver le DU de l'identité quand son elle perd son statut de membre.
- Retirer un
Copyparce que je n'étais pas sûr de modifier l'original ou une copie.
fixes #272 (closed)
Merge request reports
Activity
changed milestone to %runtime-1000
added RN-runtime label
requested review from @HugoTrentesaux
added 2 commits
59 59 ) -> Result<R, E> { 60 60 pallet_identity::Pallet::<T>::try_mutate_exists(key, |maybe_idty_data| { 61 61 if let Some(ref mut idty_data) = maybe_idty_data { 62 let mut maybe_first_eligible_ud = Some(idty_data.first_eligible_ud); 62 let mut maybe_first_eligible_ud = Some(idty_data.first_eligible_ud.clone()); 63 63 let result = f(&mut maybe_first_eligible_ud)?; 64 64 if let Some(first_eligible_ud) = maybe_first_eligible_ud { 65 65 idty_data.first_eligible_ud = first_eligible_ud; 66 66 } 67 67 Ok(result) - Comment on lines -62 to 67
I find this code a bit difficult to read. I agree that the removal of
Copyincreases readability a bit, may this be improved further? It is not clear that havingmaybe_first_eligible_udbecomeNoneafter call off()does not make sense and that this will not update the value ofidty_data.first_eligible_ud.We could:
- remove the
FirstEligibleUdtyping layer - make
first_eligible_udfield ofIdtyDatabe directly aOption<UdIndex>
Not sure it's worth it though.
We can simply add a note:
62 let mut maybe_first_eligible_ud = Some(idty_data.first_eligible_ud.clone()); 63 let result = f(&mut maybe_first_eligible_ud)?; 64 if let Some(first_eligible_ud) = maybe_first_eligible_ud { 65 idty_data.first_eligible_ud = first_eligible_ud; 66 } 67 Ok(result) 62 let mut maybe_first_eligible_ud = Some(idty_data.first_eligible_ud.clone()); 63 let result = f(&mut maybe_first_eligible_ud)?; 64 if let Some(first_eligible_ud) = maybe_first_eligible_ud { 65 idty_data.first_eligible_ud = first_eligible_ud; 66 } 67 // a f() mutating maybe_first_eligible_ud to None would not make sense 68 Ok(result) - remove the
-
Je ne sais pas à quel point la couche de typage
FirstEligibleUdnous aide ou nous embrouille. Il faudrait également ajouter un live test qui vérifie quefirst_eligible_ud.0vautSomepour lesIdtyStatus::Memberet seulement eux.Ce test devrait échouer pour la gdev car maintenant le storage est incohérent.
-
Je m'aperçois que la différence entre
IdtyData::new()etIdtyData::default()est importante : le premier est initialisé avec un premier DU éligible au genesis alors que le second ne peut pas réclamer de DU.Il faut fait attention à ne pas initialiser une identité non membre avec
newsous peine qu'elle puisse réclamer un DU. Donc il doit y avoir un deuxième bug où les identités non membres du genesis ont droit au DU.→ oui, autre bug critique #273 (closed)
Juste une remarque sur la forme (pour les prochaines fois) : pour faciliter la relecture, découper en deux commits : un pour le test (qui échoue) et un pour le correctif (où le test passe). C'est une façon de faire assez répandue, je la pratiquais sur Duniter v1 et Eloïs m'a rapporté que c'était aussi la façon de faire chez Moonbeam par exemple.
Pour le relecteur il suffit alors de vérifier la CI ou bien de se positionner sur le commit avec le test pour vérifier que le test fait bien ce qui est attendu. Là j'ai dû manipuler le code pour faire la vérification, alors que ça ne coûte pas grand-chose de le faire au commit.
added 2 commits
added 3 commits
-
853c81f9...14b9f289 - 2 commits from branch
master - d836b5f8 - fix: disable UD when membership removed
-
853c81f9...14b9f289 - 2 commits from branch
added 2 commits
mentioned in commit d17dcd65
