feat(rust/catalyst-types): change RoleIndex to enum

[vlopes11]

Description

This commit introduces RoleIndex as enum for a better static control over the logical variants of the type.

Related Issue(s)

closes #277

Description of Changes

• Change RoleIndex from RoleIndex(u16) to enum

Breaking Changes

• Change RoleIndex from RoleIndex(u16) to enum

Screenshots

N/A

Related Pull Requests

N/A

Please confirm the following checks

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream module

[github-actions]

✅ Test Report | ${\color{lightgreen}Pass: 318/318}$ | ${\color{red}Fail: 0/318}$ |

[Mr-Leshiy]

Missing this part from the issue Replace the usage of the RoleNumber type with the new RoleId..
RoleNumber structure is defined inside the rbac-registration crate. And this type facilitates exactly the same purpose, so it's redundant to have two types for the same entity.

So you will need replace the RoleNumber with the RoleId inside the rbac-registration crate and remove RoleNumber at all.

[Mr-Leshiy]

Despite one comment LGTM.
Only one more thing before we will merge this PR into main.
Pls create a PR with the updating and applying this branch to the cat-gateway first, and if everything would be fine on the cat-gateway side will merge this PR.

[stevenj]

When working with enums in rust, this crate:
https://docs.rs/strum_macros/0.27.1/strum_macros/derive.EnumDiscriminants.html
which we already use can save a lot of boilerplate and maintenance keeping everything in-sync.

Suggest looking to see if anything other than the two place I highlight could be simplified using these derived macros.

[bkioshn]

Make sure that comment related to "Role Index" and "Role number" are changed

eg. This should change to Role ID

/// Role Index is not encoded correctly
    InvalidRoleId(#[from] RoleIdError),

[stevenj]

I feel like this enum should have a method that returns True if the RoleId is a known variant, and false otherwise.
Which could be used by the currently removed validate_role_numbers function below to determine if any particular role number is known without worrying about what exact role number it is.

You could use: https://docs.rs/strum/latest/strum/derive.EnumIter.html
or https://docs.rs/strum/latest/strum/derive.VariantArray.html
to achieve this without having to update the method when we add new variants.

[vlopes11]

The initial rationale was that the static list of known roles were valid. Is the valid list a subset? The previous implementation of validate_role_numbers contemplated only roles 0 and 3; do we have a documented list somewhere of what is a valid role? For me to link here for future reference

[Mr-Leshiy]

Here is the original spec of the definition of the different role id types https://github.com/input-output-hk/catalyst-CIPs/blob/x509-catalyst-role-definitions/CIP-XXXX/README.md#project-catalyst-user-role-registrations

[stevenj]

Remove VoterDelegation we are not going to use this role at this number.
So the known members are 0,1 and 3 only so far.

[Mr-Leshiy]

Better replace this with using this derive macro https://docs.rs/strum/latest/strum/derive.EnumIs.html

[vlopes11]

Since this is a group matching, and EnumIs matches individually, I wonder if that would be an improvement. We can hardly beat the performance and simplicity of matches.

With matches, the macro will expand to a single pattern matching. With EnumIs, we would have to do something like self.is_role0() || self.is_delegated_representative() || self.is_proposer(), which would be 3 matching + ors. The compiler might be able to optimize this, but it would be more verbose.

[Mr-Leshiy]

No no, I've meant to not have an explicit is_known method defined by ourselves at all.
rather to use EnumIs so it will define for us something like is_unknown which would be more than enough to call !is_unknown in any other place.

[Mr-Leshiy]

No no, I've meant to not have an explicit is_known method defined by ourselves at all.
rather to use EnumIs so it will define for us something like is_unknown which would be more than enough to call !is_unknown in any other place.

[Mr-Leshiy]

Suggested change

proptest! {

[Mr-Leshiy]

Suggested change

use proptest::prelude::*;