Nits #52
Nits #52
Conversation
| @@ -83,7 +84,7 @@ func (s AllowListRole) IsAdmin() bool { | |||
| // IsEnabled returns true if [s] indicates that it has permission to access the resource. | |||
| func (s AllowListRole) IsEnabled() bool { | |||
| switch s { | |||
| case AllowListEnabled: | |||
| case AllowListAdmin, AllowListEnabled: | |||
There was a problem hiding this comment.
I feel like this is kind-of misleading since both of role name and function contains Enabled however the function returns true for Admin as well. Plus do we really want admins to be able to mint tokens?
There was a problem hiding this comment.
Admin is a strictly higher and should be considered enabled as well. If we don't consider it enabled, then the UX for an admin will be that they have to create a new address, mark it as enabled, and then mint funds from that address. It's much simpler if we just treat Admin as a strictly higher privilege level.
| }, | ||
| "mint big": { |
There was a problem hiding this comment.
why have we removed this test-case? I'd prefer to test mint capabilities here instead of allowList capabilities. (or preferably both of them).
There was a problem hiding this comment.
Added it back in. I didn't think this was really different than the other minting test case.
| @@ -153,7 +154,7 @@ func createAllowListRoleSetter(precompileAddr common.Address, role AllowListRole | |||
| modifyAddress := common.BytesToAddress(input) | |||
|
|
|||
| if readOnly { | |||
| return nil, remainingGas, ErrWriteProtection | |||
| return nil, remainingGas, vmerrs.ErrWriteProtection | |||
There was a problem hiding this comment.
ooh seems I imported go-ethereum vm package, instead of subnet-evm. that's why it worked haha
There was a problem hiding this comment.
but certainly not a bad idea? :P
There was a problem hiding this comment.
I think we should migrate to a separate package. We should not rely on any geth packages where we also have the same package present in our code.
| // You should have received a copy of the GNU Lesser General Public License | ||
| // along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package vmerrs |
There was a problem hiding this comment.
should we rename this to error as it's actually not in the vm package? or maybe at least vmerrors?
There was a problem hiding this comment.
alternatively we can try to import VM package from geth for errors? it won't be a clean way but still works, right?
There was a problem hiding this comment.
Currently this is specific to vm errors, I'd be alright with renaming it to vmerrors, but prefer the shortened vmerrs tbh.
Same comment on importing the VM package as above.
Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org>
This PR: