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: allow multiple operators #16

Merged
merged 8 commits into from
Mar 6, 2024

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Feb 19, 2024

This PR allows the Core contract to register multiple operators.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 19, 2024

@glihm hey :)

This is a first draft!
I used a legacyMap as specified, but i think my implementation is not optimized.

Indeed, get_operator is more like a is_operator function, returning true if the address passed to the function is an operator.
Maybe should we use some array to store all operators in the contract?

@TAdev0 TAdev0 marked this pull request as draft February 19, 2024 17:47
@glihm
Copy link
Collaborator

glihm commented Feb 19, 2024

@glihm hey :)

This is a first draft! I used a legacyMap as specified, but i think my implementation is not optimized.

Indeed, get_operator is more like a is_operator function, returning true if the address passed to the function is an operator. Maybe should we use some array to store all operators in the contract?

Hey, thanks for the progress!
Hum, actually the very first objective of the hashmap is to check. So we're totally right and The solidity code is actually using is_operator so you can totally rename. 👍
They are also using register_operator instead of set_operator. You can also rename it will be better.

Those names was because the first impl didn't add the support for multiple operators. 👍

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Will wait the renaming for other review, good progress. 👍

src/config/component.cairo Outdated Show resolved Hide resolved
@TAdev0 TAdev0 marked this pull request as ready for review February 19, 2024 19:31
@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 19, 2024

@glihm PR updated :)

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Some minor comment on formatting, and we should be good to go!

src/config/interface.cairo Outdated Show resolved Hide resolved
src/config/interface.cairo Outdated Show resolved Hide resolved
src/config/interface.cairo Show resolved Hide resolved
src/config/tests/test_config.cairo Outdated Show resolved Hide resolved
src/config/tests/test_config.cairo Outdated Show resolved Hide resolved
@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 22, 2024

@glihm PR updated !
I changed the error message in tests for "expect not operator". Tell me if you prefer something else, like "operator already registered" ..

@TAdev0 TAdev0 requested a review from glihm February 25, 2024 19:13
Copy link
Contributor

@b-j-roberts b-j-roberts left a comment

Choose a reason for hiding this comment

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

I left a couple of things, thank you!

src/config/component.cairo Outdated Show resolved Hide resolved
src/config/interface.cairo Show resolved Hide resolved
@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 26, 2024

hi @b-j-roberts , i just updated the PR. I also added a check for both the register and unregister functions, and added a test for the unregister function.
Tell me if this is ok for you.

Copy link
Contributor

@b-j-roberts b-j-roberts left a comment

Choose a reason for hiding this comment

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

Great work! Thank you.

@b-j-roberts b-j-roberts merged commit 0d8af6e into keep-starknet-strange:main Mar 6, 2024
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.

3 participants