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

Clarify function naming as it relates to ACL restrictions - change addAdmin to createAdmin, address comments #184

Open
sambacha opened this issue Sep 16, 2023 · 1 comment
Assignees
Labels
ℹ️ Discussion Informative Issue used for pre-planning / gathering requirements / etc

Comments

@sambacha
Copy link
Contributor

remove or delete?1

function addAdmin(address newAdmin) external onlyAdmin {

When the difference between remove and delete is not so obvious to you, I’d suggest looking at their opposite actions - add and create.
The key difference between add and create is that add needs a destination, while create requires no destination.

You add an item to somewhere, but you don’t “create it to somewhere”.

Simply pair remove with add and delete with create.

Suggestion

Change the wording from addAdmin to createAdmin.

Further Discussion

This is related to the issue, but not necessarily needed to resolve and triage for this issue to be completed.

Access Control List details

This highlights an additional issue of the ordering of operations in this ACL scheme that is meant to restrict certain operations to certain addresses.

We may want to implement a 'base' ACL ring (that only serves as a registration method, it does not grant any permissions to the protocol). An example: https://github.com/transmissions11/solmate/blob/fab107565a51674f3a3b5bfdaacc67f6179b1a9b/src/auth/Trust.sol

Accounts should be 'Trusted'. Then elevate the permissions to the respective roles. We should add a hard limit to the number of active addresses assigned to their relative roles.

Consider: EIP-173 Contract Ownership Standard2


See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-173.md

Example implementation for reference: https://github.com/Nipol/bean-contracts/blob/main/contracts/library/Ownership.sol

Note
The Bean Contracts repo has a bespoke authorization and permissioning setup, see the contract Wizard for the spell book ref. It is in Korean.

Reference

Footnotes

  1. See Manifold Finance Styling Guide, Naming Things is Easy: Delete

  2. See this handy tool, EIP-FZF https://github.com/cds-amal/fzf-eip

@sambacha sambacha added the ℹ️ Discussion Informative Issue used for pre-planning / gathering requirements / etc label Sep 16, 2023
@sambacha
Copy link
Contributor Author

I only mention fzf eip because there are so many god damn EIPs its hard to keep up with them all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ℹ️ Discussion Informative Issue used for pre-planning / gathering requirements / etc
Projects
None yet
Development

No branches or pull requests

3 participants