À quoi servent les pending membership?

Protocols Ğ1v2 protocol
15

Pour moi, la notion d’identité et la notion d’adhésion sont différentes, donc mieux vaut les séparer dans des pallets testables indépendamment. Il n’y a pas besoin d’une pallet dédiée, on pourrait tout mettre dans une seule pallet, mais ça me semble moins clair, c’est tout.

On pourrait déplacer les calls claim_membership et renew_membership dans la pallet identity, déplacer le storage Membership et MembershipExpireOn dans la pallet identity, ou même supprimer le storage Membership pour le mettre dans un champ de IdtyValue… Tout ça est possible oui, c’est juste un choix d’organisation. Je trouve ça plus clair quand c’est organisé dans des petits modules, tu trouve ça plus clair quand c’est organisé dans moins de modules (donc plus gros). Je propose un refactoring qui élague, tu proposes un refactoring qui élague et regroupe des pallets.

Je ne pense pas que ce soit très productif qu’on travaille en parallèle sur les deux solutions pour n’en retenir qu’une à la fin, donc je vais laisser ça de côté, y revenir à tête reposée, éventuellement en parler avec d’autres (comme @poka, @tuxmain, @bgallois, @vit, @kimamila), qui auront peut-être également un avis, et voir ce qui plait le plus.

16

Tu bottes en touche à chaque fois.

Je répète : quand tu enlèves les call que tu comptes enlever, que reste-t-il ? As-tu fait l’inventaire avant de me répondre ?

On peut aussi créer une pallet pour chaque micro-donnée, ça va être bien modularisé, vraiment ?

17

Il restera :

  • deux calls
    • claim_membership
    • renew_membership
  • pas mal de fonctions internes
    • force_expire_membership
    • try_claim_membership
    • unschedule_membership_expiry
    • insert_membership_and_schedule_expiry
    • check_allowed_to_claim
    • do_claim_membership
    • do_expire_membership
    • expire_memberships
  • deux éléments de storage
    • Membership
    • MembershipsExpireOn
  • trois événements
    • MembershipAcquired
    • MembershipExpired
    • MembershipRenewed
  • quatre erreurs
    • IdtyIdNotFound
    • MembershipAlreadyAcquired
    • MembershipNotFound

Soit à peu près 300 lignes de code. lib.rs de la pallet identity fait 800 lignes de code. Réunir les deux nous fait passer au dessus des 1000 lignes de code. On pourrait aussi séparer en différents fichiers, mais pour la facilité de compréhension et de gestion du code, je préfère toujours avec deux pallets. Peut être qu’en revenant plus tard j’aurai changé d’avis, mais pour l’instant c’est ce que je préfère. J’aime bien la séparation en pallets :

  • identity
  • membership
  • wot
  • certification

J’aime moins la séparation en pallet sans membership :

  • identity
  • wot
  • certification

C’est pas argumenté, c’est juste une histoire de préférence d’organisation.

18

Voilà. Et encore, si tu détailles les éléments du storage : Membership ne fait que stocker le bloc d’expiration. Quant à MembershipsExpireOn, c’est la transposée.

Bref, une seule donnée à stocker par identité. On garde une pallet pour ça ? On explique aux clients qu’il faut un concept supplémentaire juste pour une date d’expiration ? Pour le coup on va dupliquer des clés (idty_id) dans le storage “par principe de séparation des problèmes”. Pour moi ce n’est pas la raison qui parle, c’est du dogme (ou de le résistance au changement, je n’en sais rien).

Cela dit, je ne vais pas insister. Je trouve déjà que les suppressions que tu proposes sont une bonne avancée. :+1:

D’un point de vue du protocole (et des tests), ce sera déjà plus simple.

Bref je vote pour tes propositions. :white_check_mark:

1 Like
19

Le révocation de forgeron peut servir à quelqu’un qui sait qu’il ne fera plus goOnline et ne veut plus être soumis à un risque aussi élevé en cas de fuite de clé privée. Sans ça, l’ex forgeron est obligé de se conformer à la licence forgeron, même s’il n’a plus les moyens de la respecter, jusqu’à expiration.

Pas besoin de dupliquer des clés si on n’a qu’une seule StorageMap pour les identités, contenant dans un champ une struct membership. Ça alourdit la config avec des providers mais c’est du code creux.

20

J’ai pas très bien dormi cette nuit, mais je me suis quand même réveillé avec une idée en plus :

  • lorsque le nombre de certifications reçues passe en dessous du seuil, il faut supprimer l’adhésion, que ce soit pour la toile principale, ou la toile foreron

Actuellement c’est fait avec le handler OnRemovedCert dont l’implémentation par la pallet wot est la suivante :

// remove cert handler
impl<T: Config<I>, I: 'static> pallet_certification::traits::OnRemovedCert<IdtyIndex>
    for Pallet<T, I>
{
    fn on_removed_cert(
        _issuer: IdtyIndex,
        _issuer_issued_count: u32,
        receiver: IdtyIndex,
        receiver_received_count: u32,
        _expiration: bool,
    ) {
        if receiver_received_count < T::MinCertForMembership::get()
            && pallet_membership::Pallet::<T, I>::is_member(&receiver)
        {
            // expire receiver membership
            // it gives them a bit of time to get back enough certs
            if let Err(e) = <pallet_membership::Pallet<T, I>>::force_expire_membership(receiver) {
                sp_std::if_std! {
                    println!("fail to expire membership: {:?}", e)
                }
            }
        }
    }
}

Si on retire la pallet membership, il faudra déplacer cette implémentation dans la pallet identity pour les adhésions normales et dans la pallet authority members pour les adhésions forgeron. Cette nuit cette idée ne me plaisait pas, mais en l’écrivant, ça ne me gène pas plus que ça. L’idée fait son chemin :slight_smile:

Effectivement, dit comme ça, ça donne envie de faire le ménage !!

Plutôt de la résistance au changement, mais pour ça, la nuit peut aider (alors que pour le dogme ça n’y fait pas grand chose).

Pas besoin d’insister, juste amener des arguments et des idées, et laisser un peu de temps à ma petite tête pour digérer tout ça. D’où le :

Actuellement, la révocation d’adhésion forgeron n’empêche pas quelqu’un qui récupère la clé de refaire request_membership et claim_membership, les certifications sont toujours valides. Si on veut obtenir cet effet de révocation longue durée, il faut soit bannir les clés soit pouvoir annuler des certifications forgeron reçues.

21

Euh, ces palettes ne connaissent pas les certifs, tu veux parler de duniter-wot ?

Il faut en effet supprimer les certifs forgeron et choisir un délai après révocation (même mécanisme que pour la révocation principale). Mais le délai devrait être plus court ici.

Par contre si le forgeron revient il faut s’assurer qu’il a changé ses clés de session. Avec une table de clés bannies ? (et donc une expiration des bannissements au bout de N années…)

22

En fait non, je me suis mal exprimé, on garde l’implémentation dans la pallet wot, mais la fonction qu’elle appelle sera dans une autre pallet.

// cette partie reste dans la pallet wot
if receiver_received_count < T::MinCertForMembership::get()
// cette partie disparaît, pas besoin de vérifier si l'identité est membre pour transmettre
// le message qu'elle est passée en dessous du seuil de certifications
&& pallet_membership::Pallet::<T, I>::is_member(&receiver)
// cette partie est déplacée dans les pallets qui vont gérer les adhésions
if let Err(e) = <pallet_membership::Pallet<T, I>>::force_expire_membership(receiver) {

Donc en gros, ça fait

if receiver_received_count < MinCertForMembership
    try_remove_membership()

Pour l’instance wot, ce sera la pallet identity qui essaiera de retirer l’adhésion (mais si elle n’est déjà plus là il n’y a rien à faire), pour l’instance smith_wot, ce sera la pallet authoriy_members.

23

Les idty_id sont des u32 (32 bits), pas des clés publiques (32 octets). Donc l’impact de la duplication est moindre. Mais effectivement, ça fait de la duplication peu utile.

24

Dans un premier temps ça peut être fait comme ça, et dans un 2ème temps : on peut carrément faire sauter la pallet duniter-wot.

Ce n’est pas que je veux tout faire sauter ! Mais ça permet de découpler la partie WoT de la partie autorités, vu que cette idée de Smith WoT ne te satisfait pas vraiment et que cet avis est partagé par d’autres (moi le 1er). La solution d’Eloïs avait le mérite de fonctionner tout de suite, mais si l’on peut faire plus adapté (et surtout moins complexe), ça peut valoir le coup.

A mon avis, authority_members pourrait être auto-suffisante pour gérer la problématique d’entrée/sortie des Smith, avec des règles plus spécifiques que celles de la WoT classique. Il y aura sûrement un lien avec la pallet identity, mais pas beaucoup plus.

Actuellement je ne suis pas serein sur les tests, je trouve que la combinatoire est trop grande avec tous les liens qui existent au niveau des pallet de WoT, qui en plus est dupliquée avec les Smith. Je pense que stratégiquement il vaudrait mieux séparer tout ça.

1 Like
25

Actuellement il y a des palettes très génériques (membership, identity, cert) et une palette “colle”, duniter-wot. Qui ferait son travail ?

Idée pour éviter les problèmes de combinaisons imprévues : implémenter une machine à états dans duniter-wot, qui décrit entièrement tous les états possibles d’une identité, et toutes les transitions entre états (aucune transition partielle). Mais ça risque d’être une usine à gaz monolithique.

2 Likes
26

Si l’on part des deux hypothèses suivantes :

  • les Smith sont gérés indépendamment via la pallet authority-members
  • le concept d’adhésion de la WoT saute

Alors la palette duniter-wot ne sert plus qu’à “coller” identiy, certification ainsi qu’un peu de distance, ce que ces palettes pourraient faire en interne sur leurs parties respectives car il resterait :

  • Constantes :
    • appliquer FirstIssuableOn : concept de certification
    • appliquer MinCertForMembership : concept de certification
    • appliquer MinCertForCreateIdtyRight : concept de certification
    • appliquer IsDistanceOk : concept de distance
  • Fonctions :
    • check_create_identity : checks de certifications
    • check_validate_identity : check réductible à l’appel de distance.requestDistanceEvaluation
    • check_cert_allowed : check de certifications
    • on_idty_change.IdentityEvent::(Created|Removed) : check de certifications
    • on_new_cert : event de certifications
    • on_removed_cert : event de certifications

En fait, en l’écrivant, je me rends compte que 90% du code restant sont des traitements liés aux certifications. Quant à la vérification de distance, c’est un appel direct à la pallet de distance, il n’y a pas de code à déplacer, juste à laisser les palettes identity et distance se parler.

2 Likes
27

@HugoTrentesaux pour clarifier ma position : je ne compte pas refactorer quoi que ce soit pour l’instant. Si tu veux tu peux déjà t’occuper des PendingMembership comme tu souhaitais le faire. Tu peux y aller, nous ne nous marcherons pas dessus.

Eventuellement je verrai pour un petit POC de modification de la palette authority-members afin d’isoler la feature “Smith”, condition préalable au démantèlement des adhésions et de la palette duniter-wot. Sauf si à un moment tu me dis que finalement tu a bien envie de le faire.

1 Like
28

J’ai du mal à comprendre concrètement. La proposition est-elle :

  • coupler fortement toutes ces palettes ensemble, en distribuant le code de duniter-wot dans chacune d’elles ?
  • fusionner duniter-wot et identity ? (ce qui créerait des dépendances improbables, genre de certifications à distance)
  • n’utiliser que des fonctions génériques dans les Config et quelques providers/handlers dans runtime-common pour éviter d’avoir des fonctions ad hoc dédiées aux spécificités de la WoT et à la glu inter-palettes ?

Les deux premières solutions me font peur et je trouverais ça très dommage, la dernière me plaît mais semble plus difficile.

29

La proposition ne peut pas être ni la 1ère ni la 2ème: car duniter-wot ne ferait plus que de la certification, donc seule la palette certification serait impactée.

Mais je ne suis pas sûr que ça ressemble exactement à la 3ème proposition non plus, je ne vois pas de quelle “glue” il y aurait besoin :

  1. identity.create_identity a besoin de la palette certification, aujourd’hui abstrait par duniter-wot via CheckIdtyCallAllowed::check_create_identity
  2. identity.confirm_identity : aucun lien avec une autre palette
  3. certification.add_cert : a besoin de vérifier l’existence des identités, aujourd’hui abstrait par duniter-wot via CheckCertAllowed::check_cert_allowed
  4. distance.request_distance_evaluation : a besoin de vérifier l’existence et le statut des identités (+ je rajouterai : vérifier aussi le nombre de certifications)

Le workflow final pour devenir membre prend 4 étapes avec un workflow sans adhésions (contre 6 auparavant : il fallait demander + confirmer l’adhésion), et dedans les seuls “liens” à faire sont que identity et certification se causent, et que distance puisse avoir accès à identity et certification.

On peut parler de “glue”, je ne sais pas exactement quelle forme donner à celle-ci, mais j’ai l’impression qu’un simple couplage fort (trait Config qui implémente identity ou certification selon le cas) serait tout à fait acceptable et logique. Passer par des types comme CheckIdtyCallAllowed (comme aujourd’hui) me paraît un peu overkill, mais pourquoi pas.

1 Like
30

Pas si évident, notamment pour les tests unitaires de la palette. Passer par un handler/provider est plus souple.

1 Like
31

J’ai beaucoup avancé dans mon refac dans la MR !215, le code est devenu plus propre et compréhensible.

Et au vu de ton post Refonte de la Smith WoT et de l’intérêt que tu portes à la question, je me demande si je ne ferais pas mieux d’arrêter là dans le ticket #156 et de ne pas :

  • retirer le call revoke_membership
  • retirer l’instance smith de la pallet membership
  • apporter des modification à la pallet authority_members
  • permettre le changement de clé pour les forgerons

Comme ça j’ai fait la simplification que j’estimais indispensable et les bases sont propres pour les évolutions que tu proposes. Parmi ces nouvelles bases :

  • une distinction claire pour les statuts d’identité :
enum IdtyStatus {
    Unconfirmed,
    Unvalidated,
    Member,
    NotMember,
    Revoked,
}

  • une distinction entre la révocation d’identité et le retrait d’identité avec une cause précisée dans le message d’erreur (cf RevocationReason et RemovalReason)

  • des deadlines pour faire les actions bien définies

/// Period during which the owner can confirm the new identity.
// something like 2 days but this should be done quickly as the first certifier is helping
type ConfirmPeriod: Get<Self::BlockNumber>;
/// Period before which the identity has to be validated (become member).
// this is the 2 month period in v1
type ValidationPeriod: Get<Self::BlockNumber>;
/// Period before which an identity who lost membership is automatically revoked.
// this is the 1 year period in v1
type AutorevocationPeriod: Get<Self::BlockNumber>;
/// Period after which a revoked identity is removed and the keys are freed.
type DeletionPeriod: Get<Self::BlockNumber>;

  • un grand ménage dans les IdtyEvent qui passe de 5 à 2 cas

  • des messages d’erreurs plus précis partout (certifications, toile, membership, distance…)

enum MembershipRemovalReason {
    // reach end of life
    Expired,
    // was explicitly revoked
    Revoked,
    // received certs count passed below threshold
    NotEnoughCerts,
    // system reasons (consumers, authority members, or root)
    System,
}

  • le retrait de “validate_identity” qui n’a pas de raison d’être vu que “claim_membership” existe

  • et évidemment le retrait de pending* et request_membership

Il me reste un peu de ménage à faire notamment sur les live tests, mais sinon c’est regardable en l’état.

3 Likes
32

Je vais faire une relecture d’ici demain soir.

En effet ça ne vaut peut-être pas le coup de refactorer certaines parties touchant aux memberships et à authority_members. Ou au moins fais une pause le temps de voir concrètement la MR que je vais proposer, et voir si celle-ci convient et les impacts que tu pourras en tirer pour poursuivre ton refacto.

1 Like
33

Je voyais plutôt ça dans l’autre sens, tu bases ta branche sur la mienne. D’abord refac membership, ensuite refonte smith wot.

34

Oui c’est ça : je dis juste qu’avant d’aller plus loin, il faut soit jeter ce que je vais faire, soit le merger sur ta branche.