chore: update Allo.sol #24
Conversation
thelostone-mc
force-pushed
the
update-allo
branch
from
ec40ceb
to
3e6c3b7
Compare
thelostone-mc
force-pushed
the
update-allo
branch
6 times, most recently
from
5edd588
to
f2b4801
Compare
thelostone-mc
force-pushed
the
update-registry
branch
4 times, most recently
from
8bba45c
to
ed12f40
Compare
thelostone-mc
force-pushed
the
update-allo
branch
2 times, most recently
from
8405f8c
to
b786c82
Compare
thelostone-mc
force-pushed
the
update-allo
branch
from
b786c82
to
02d9992
Compare
thelostone-mc
force-pushed
the
update-registry
branch
from
dadaec7
to
687e088
Compare
thelostone-mc
force-pushed
the
update-allo
branch
2 times, most recently
from
355794a
to
5566079
Compare
thelostone-mc
force-pushed
the
update-registry
branch
from
b5f3c4e
to
ef1eea0
Compare
thelostone-mc
force-pushed
the
update-allo
branch
from
7d5ce5b
to
394dc5a
Compare
thelostone-mc
force-pushed
the
update-allo
branch
from
ae33870
to
950dd1d
Compare
| // grant pool owners roles | ||
| for (uint256 i = 0; i < _owners.length;) { | ||
| _grantRole(POOL_OWNER_ROLE, _owners[i]); | ||
| unchecked { | ||
| i++; | ||
| } | ||
| } | ||
|
|
||
| // todo: return the poolId? what do we want to return here? | ||
| return _poolId; | ||
| // grant admin roles to pool creator | ||
| _grantRole(POOL_ADMIN_ROLE, msg.sender); | ||
| // set admin role for POOL_OWNER_ROLE | ||
| _setRoleAdmin(POOL_OWNER_ROLE, POOL_ADMIN_ROLE); |
There was a problem hiding this comment.
Personally I think I'd name these opposite? It doesn't really matter but I think of admins as all the people who can control the pool, and the owner is the one who actually owns it and has ultimate say.
There was a problem hiding this comment.
You thinking something like this:
// grant pool admin roles
for (uint256 i = 0; i < _admins.length;) {
_grantRole(POOL_ADMIN_ROLE, _admins[i]);
unchecked {
i++;
}
}
// grant owner role to pool creator
_grantRole(POOL_OWNER_ROLE, msg.sender);
// Note: do we want this? set admin role for pool creator
// _grantRole(POOL_ADMIN_ROLE, msg.sender);There was a problem hiding this comment.
Yup we've changes this to better naming convention on the strategy PR. It's called admin and pool manager
| // DISCUSS: We either | ||
| // - allow cloning of every contract (use the bool) | ||
| // - or we force cloning of only approved contracts |
There was a problem hiding this comment.
Is this DISCUSS still relevant or is it old? I think current implementation here is perfect, but if there's still question, let's talk about it.
There was a problem hiding this comment.
This still needs to be discussed cause the boolean arguments don't make a lot of sense at this point the way the contract is written. Looking at the code you realise, that the bool is in theory a dummy argument cause :
- if you pass bool as true but the strategy is not approved -> it reverts
- if you pass bool as true but the strategy is approved -> it clones
- if you pass bool as false but the strategy is approved -> it reverts
- if you pass bool as false but the strategy is not approved -> it doesn't clone
This feels the the same as
- if contract is approved -> it clones
- if contract Is not approved -> it doesn't clone
The boolean is added just to share more clarity on reverts which I'm not sure if worth it (i could be wrong but hence the discussion )
thelostone-mc
force-pushed
the
update-allo
branch
from
5a3b402
to
4b17e64
Compare
|
@zobront Merging this off so that we can proceed with the strategies PR Things to verify : |
No description provided.