IdtyEvent duplicates pallet-identity::Event

Support Duniter-v2
1

I’ve noticed this duplication while crawling the Event type across our pallets.

I added an issue Identity pallet events are duplicated (#115) · Issues · nodes / rust / Duniter v2S · GitLab to track it. I also pushed a branch fix/idty-events-duplicate branch trying to solve it, but then I had to add theses lines (here and there):

_ => {} // TODO: why is it necessary?

The compiler seems to be unable to guarantee that our match idty_event matches all patterns without this instruction.
Of course, to me, it does cover all the patterns. There must be some Rust subtility here.

Do you have an idea why this error arise?

Afaik, it is the second and only duplication of Event in Duniter V2S. The other being pallet-membership::Event duplication under primitives/.

2

It is indeed exhaustive if we use only safe code, but another invalid variant can be constructed in unsafe code without UB, because the match takes a reference. see this discussion

The solutions are to dereference or to clone.

I didn’t know about this, it’s quite surprising but it makes some sense.

3

Look ta both examples: one is without reference.

I will try to dig the mentionned thread, though.

4

Both take idty_event: &pallet_identity::Event<T>. The last pattern can be _ or &_ because the two will match any idty_event.

5

Sorry I miscommited. I’ve just pushed the version removing the references usage in the match expression.

The problem remains the same: the compiler estimates the match does not cover all the existing patterns.

error[E0004]: non-exhaustive patterns: `_` not covered
   --> pallets/duniter-wot/src/lib.rs:305:15
    |
305 |         match idty_event {
    |               ^^^^^^^^^^ pattern `_` not covered

edit: it seems that we have exactly the same problem with Membership events. All the places using:

match membership_event {

are actually using the sp_membership::Event enum. The compiler error I encounter might be the reason why Eloïs made a copy of the events under the primitives crate.

6

MR!173

Explanation

To understand what was going on, I wrote this minimal test:



#[test]
fn test_match() {
    let event: Event<Test> = Event::IdtyCreated {
        creator: 0,
        idty_index: 0,
        owner_key: account(0).id,
    };

    match event {
        Event::IdtyCreated { .. } => {}
        Event::IdtyConfirmed { .. } => {}
        Event::IdtyValidated { .. } => {}
        Event::IdtyChangedOwnerKey { .. } => {}
        Event::IdtyRemoved { .. } => {}
    }

    assert_eq!(true, true)
}

And the compiler was more talkative:

Capture d’écran 2023-06-01 à 13.25.39
Capture d’écran 2023-06-01 à 13.25.391002×548 37.2 KB

So the #[pallet::event] macro adds an __Ignore variant on every Pallet Event.

I just needed to replace:

  • _ => {}
  • by pallet_identity::Event::__Ignore(_, _) => {}

Which allows use to get back the exhaustive matching pattern guarantee of Rust, which will raise an error if we add an Event and forget to handle it somewhere. :slight_smile:

4 Likes
7

It reminds me about what I wrote here: Undocumented Duniter concepts - #7 by HugoTrentesaux

But now I understood the interest of having this apparent redundancy.
If we look at the two occurrences:

in types.rs:

/// events related to identity
pub enum IdtyEvent<T: crate::Config> {
    /// creation of a new identity by an other
    Created { creator: T::IdtyIndex },
    /// confirmation of an identity (with a given name)
    Confirmed,
    /// validation of an identity
    Validated,
    /// changing the owner key of the identity
    ChangedOwnerKey { new_owner_key: T::AccountId },
    /// removing an identity
    Removed { status: IdtyStatus },
}

in lib.rs

#[pallet::event]
    #[pallet::generate_deposit(pub(super) fn deposit_event)]
    pub enum Event<T: Config> {
        /// A new identity has been created
        /// [idty_index, owner_key]
        IdtyCreated {
            idty_index: T::IdtyIndex,
            owner_key: T::AccountId,
        },
        /// An identity has been confirmed by its owner
        /// [idty_index, owner_key, name]
        IdtyConfirmed {
            idty_index: T::IdtyIndex,
            owner_key: T::AccountId,
            name: IdtyName,
        },
        /// An identity has been validated
        /// [idty_index]
        IdtyValidated { idty_index: T::IdtyIndex },
        IdtyChangedOwnerKey {
            idty_index: T::IdtyIndex,
            new_owner_key: T::AccountId,
        },
        /// An identity has been removed
        /// [idty_index]
        IdtyRemoved { idty_index: T::IdtyIndex },
    }

As we can see, these events are not totally similar. The first one are used internally for modules to cooperate together while the second one are here to inform the outside world about what is happening on chain.

I think blockchain events should never be used internally because we want to be able to change them easily if we find out client need more information. On the opposite, internal events should only be modified when it is necessary for other pallets to know a bit more about what happened in an other pallet.

Sorry for the delay, I was in Britain to work with @flodef on the indexer and gcli.

4 Likes
8

Yes your answer makes sense, and so is tuxmain’s one.

And it’s also written in Substrate’s documentation :

A pallet can emit events when it wants to send notification about changes or conditions in the runtime to external entities like users, chain explorers, or dApps.

1 Like