manifoldfinance / mevETH2

mevETH LST Protocol - Repo has migrated see link
https://github.com/MEV-Protocol/meveth
27 stars 2 forks source link

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

Open sambacha opened 1 year ago

sambacha commented 1 year ago

remove or delete?[^1]

https://github.com/manifoldfinance/mevETH2/blob/216fe89b4b259aa768c698247b6facac9d08597e/src/libraries/Auth.sol#L52

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 Standard[^2]


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

[^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 commented 1 year ago

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