solidstate-solidity icon indicating copy to clipboard operation
solidstate-solidity copied to clipboard

Published 20 hours ago • verified publisher icon solidstate-network

added AccessControl

Open iam-dev opened this issue 2 years ago • 11 comments

Test need to be done

iam-dev avatar May 10 '22 19:05 iam-dev

btw where can I change the @solidstate/spec? I would like to add describeBehaviorOfAccessControl

iam-dev avatar May 10 '22 19:05 iam-dev

That package is located in the spec directory of this repository.

ItsNickBarry avatar May 10 '22 20:05 ItsNickBarry

please add describeBehaviorOfAccessControl to @solidstate/spec

iam-dev avatar May 10 '22 22:05 iam-dev

please add describeBehaviorOfAccessControl to @solidstate/spec

I don't think I have access to your branch. You can add it by importing it into spec/access/index.ts.

ItsNickBarry avatar May 11 '22 02:05 ItsNickBarry

Love that this is seeing progress, going to be watching this

friskyfoxdk avatar May 14 '22 12:05 friskyfoxdk

I will review again soon. Not much time at the moment. It appears to be nearly finished, though.

ItsNickBarry avatar May 14 '22 18:05 ItsNickBarry

@iam-dev Merge conflict fixed, pull changes.

ItsNickBarry avatar May 30 '22 21:05 ItsNickBarry

please check and merge

iam-dev avatar Jun 09 '22 08:06 iam-dev

+1 for having this support for the access control functions!

nedgar avatar Jun 20 '22 19:06 nedgar

Please forgive the delay, will get back to this as soon as I can.

ItsNickBarry avatar Jun 21 '22 22:06 ItsNickBarry

+1 Looking forward to seeing it in the next version

LorranSutter avatar Jun 30 '22 05:06 LorranSutter

Any update on when this will be merged or is there still work that needs to be done?

jhubbardsf avatar Sep 15 '22 17:09 jhubbardsf

Once again, I apologize for the delay. I've made some changes, and I believe this is almost ready to merge.

One outstanding question: why shouldn't grantRole revert if a user already has a role, and why shouldn't revokeRoke revert if a user doesn't have a role? I'd like to make one of the following changes:

  1. Revert if either of these functions is called but has no effect.
  2. Add a bool return argument to these functions to indicate whether the call had an effect.
  3. Remove the internal success checks and emit the events in every case (consistent with OwnableInternal#_transferOwnership).

Any opinions? I will merge once this is decided.

ItsNickBarry avatar Sep 15 '22 23:09 ItsNickBarry

OpenZeppelin's implementation doesn't revert on those things, so I guess that's just how it was ported. I'm wondering why they made the decision not to revert if it wouldn't do anything. Maybe there's an edge case I'm not seeing. Returning a bool would still give feedback but not be a "breaking" change compared to OpenZeppelin. So maybe change 2 would be my vote.

jhubbardsf avatar Sep 16 '22 01:09 jhubbardsf

Yeah, the PR matches the original OZ implementation, but the same question could also be asked there. AccessControl isn't based on an EIP, so it's okay to make improvements.

Thought of a third option which I'm leaning towards - see my edited comment above.

ItsNickBarry avatar Sep 16 '22 02:09 ItsNickBarry

For both grantRole and revokeRole checks can be performed ahead of time if a bool or revert is desirable. I like the third option or leaving it as is.

0xCourtney avatar Sep 16 '22 19:09 0xCourtney

Implemented option 3.

ItsNickBarry avatar Sep 18 '22 02:09 ItsNickBarry