Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Completed Implementation of role_store contract. #443

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

ametel01
Copy link
Contributor

Summary

Implemented role_store and added corresponding tests.

Labels

  • Feature

Issue Resolved

Resolves: #432

New Behavior

The new implementation mimics the behavior of the Solidity version but avoids using arrays in storage. Instead, role_members_count and roles_count track the total number of writes rather than the actual instance count. This is because deleting a <key, value> pair sets the value to zero() rather than removing it. To manage storage efficiently, the algorithm checks existing keys from 0 to the current storage count during insertion. If a zero() value is found, that key is reused; otherwise, a new key-value pair is created and the key count is incremented.

Breaking Changes

None.

Additional Info

N/A

@ametel01 ametel01 force-pushed the finish-role-store branch 3 times, most recently from 67336b7 to a953dba Compare September 23, 2023 11:18
Copy link
Collaborator

@zarboq zarboq left a comment

Choose a reason for hiding this comment

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

Nice, could you just rename some test to follow our test naming rules

Like in the first test -> given_normal_conditions_when_revoke_role_then_works

@ametel01
Copy link
Contributor Author

Nice, could you just rename some test to follow our test naming rules

Like in the first test -> given_normal_conditions_when_revoke_role_then_works

renamed tests

@zarboq
Copy link
Collaborator

zarboq commented Sep 24, 2023

Sorry I wasn't clear but the test you renamed was the one with good naming :)

Rename other tests following the same pattern (given_...)

@ametel01
Copy link
Contributor Author

Sorry I wasn't clear but the test you renamed was the one with good naming :)

Rename other tests following the same pattern (given_...)

done

@zarboq zarboq merged commit ac48e57 into keep-starknet-strange:main Sep 25, 2023
3 checks passed
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.

feat: finish role_store implementation
3 participants