Skip to content

Conversation

@Huanzhang89
Copy link
Contributor

Only the check _isSelfOnboardingApproved is updated to use bytes32 for the _role parameter, other functions such as _approveSelfOnboarding still take string.

@amarinkovic
Copy link
Member

We should stick with the existing naming convention, no need for Bytes32 in variable names. It is part of the overall design, all IDs are of type bytes32 across the codebase. Naming it roleId is how it's done everywhere else, so it would keep things consistent.

@Huanzhang89
Copy link
Contributor Author

We should stick with the existing naming convention, no need for Bytes32 in variable names. It is part of the overall design, all IDs are of type bytes32 across the codebase. Naming it roleId is how it's done everywhere else, so it would keep things consistent.

Got it, I've made the change

Copy link
Member

@amarinkovic amarinkovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both _isSelfOnboardingApproved and _approveSelfOnboarding should be changed to accept the bytes32 roleId, together with the corresponding method in the facet. Original report was about the _isSelfOnboardingApproved only, but we should keep things consistent across the board.

@Huanzhang89
Copy link
Contributor Author

made the updates and the tests are all fixed aswell

Copy link
Member

@amarinkovic amarinkovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@amarinkovic amarinkovic changed the title NAYM-4-3 - update _isSelfOnboardingApproved to take role as a bytes32 instead of string NAY4-3 - update _isSelfOnboardingApproved to take role as a bytes32 instead of string Oct 2, 2024
@Huanzhang89 Huanzhang89 changed the base branch from main to dev October 2, 2024 11:32
@amarinkovic amarinkovic changed the title NAY4-3 - update _isSelfOnboardingApproved to take role as a bytes32 instead of string NAY4-3 Update _isSelfOnboardingApproved to take role as a bytes32 instead of string Oct 3, 2024
@amarinkovic amarinkovic mentioned this pull request Oct 3, 2024
@Huanzhang89 Huanzhang89 merged commit 353849f into dev Oct 3, 2024
5 checks passed
@Huanzhang89 Huanzhang89 deleted the NAYM-4-3_role_bytes_32 branch October 3, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants