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

Refactor modifiers privilege #243

Merged
merged 4 commits into from
Oct 20, 2023
Merged

Refactor modifiers privilege #243

merged 4 commits into from
Oct 20, 2023

Conversation

MerlinEgalite
Copy link
Contributor

No description provided.

@MerlinEgalite MerlinEgalite requested review from QGarchery, a team, Rubilmax and Jean-Grimal and removed request for a team October 20, 2023 13:55
src/MetaMorpho.sol Outdated Show resolved Hide resolved
@Rubilmax
Copy link
Contributor

or perhaps don't change anything and rely on transitivity of privileges because the hierarchy is a tree (which is what was chosen for the URD)

I was against at first because I thought for a moment hierarchy was not a tree (e.g. there was a privilege for the guardian but not the owner) but there's none

@MerlinEgalite
Copy link
Contributor Author

or perhaps don't change anything and rely on transitivity of privileges because the hierarchy is a tree (which is what was chosen for the URD)

I was against at first because I thought for a moment hierarchy was not a tree (e.g. there was a privilege for the guardian but not the owner) but there's none

But then it's not consistent between repos...

@Rubilmax
Copy link
Contributor

@MerlinEgalite
Copy link
Contributor Author

I don't get it, it is: https://github.com/morpho-org/universal-rewards-distributor/blob/26388ed651c0d774d74c1335c104b4a11718e3bd/src/UniversalRewardsDistributor.sol#L55

see we named it onlyUpdater and not hasUpdaterPrivilege

I would rename it in the URD as well but it's under review for now.

@julien-devatom do you have a preference?

I like hasUpdaterPrivilege because it makes things clearer

@MerlinEgalite
Copy link
Contributor Author

Other suggestion onlyCuratorRole, etc. A bit like OZ: https://docs.openzeppelin.com/contracts/4.x/access-control

@julien-devatom
Copy link
Contributor

I'm ok, you can suggest this naming inside of the security review issue related to that

Base automatically changed from refactor/is-allocator to main October 20, 2023 16:39
@MerlinEgalite MerlinEgalite merged commit 26aa145 into main Oct 20, 2023
4 checks passed
@MerlinEgalite MerlinEgalite deleted the refactor/privilege branch October 20, 2023 17:24
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