Description
1.
There's no need to go for solidity 0.8.17 as this project is a new one. You can at least use 0.8.22 which brings a new feature where you don't actually need to use unchecked keyword anymore. Read https://soliditylang.org/blog/2023/10/25/solidity-0.8.22-release-announcement and then change wherever you use it. This should be achievable now since DaoAuthorizable and IPermissionCondition contracts now have ^ in front of their versions, allowing it to compile with 0.8.22.
2
bytes32 immutable MANAGE_SELECTORS_PERMISSION_ID =
keccak256("MANAGE_SELECTORS_PERMISSION");This can directly be constant. immutable has a different usage which is not this.
3
allowSelector can be re-written so that it expects address _target and bytes4[] selectors(note the array sign). This is better to avoid calling this function multiple times later on or you can just be passing InitialTarget[]. Note that this is ideal, because in the constructor, you wouldn't need to de-duplicate the code of setting target/selectors. What you should do is create a private function and call it from constructor as well as from allowSelectors function. Note that events also must emit after contract deployment because constructor is called in which you also set the target/selectors. This is why private function is a good idea to handle this.
4
Inheritance chain is not up to best practice. The reason being is DaoAuthorizable must come before IPermissionCondition. As solidity's graph building works, you should define chain as from most base to most derived. Just raising this for best practices, not a problem in this case, because it's highly unlikely that DaoAuthorizable ever would need to inherit from IPermissionCondition. If it did, your code would stop working.
5
Probably better to use IExecutor.execute.selector because we moved it from IDAO to IExecutor in 1.4
6
Inside every funciton, instead of actual code, I'd call internal functions. These internal functions would have those code that you have in these public functions. Then, I'd make these internal functions virtual. Not only that, the public functions must also be virtual. Not only that, none of these functions(especially isGranted function) must be external, but public.
This allows me to write a new condition contract(with extra logic) and still use your contracts as bases.
I think every one should start writing contracts with virtual keywords(when it's not gonna bring danger, why not do it) to get better at composability.
7
The error objects can receive bytes4 selector and target. When you revert, this will tell us for which selector it was erred out. Pretty helpful in practice.