Feature/Adding RoleAdminChanged event in AccessControl

[julianmrodri]

#2210

Fixes #2210

Adding a new event named "RoleAdminChanged" when the adminRole of a Role is changed. This event is emitted from with the _setRoleAdmin function.

[julianmrodri]

I see 2 potential changes worth discussing:

• I put the Declaration of the new event before the existing ones, we can move it to the after the other two events.
• Should we only trigger the event in the case the new Role Admin is different than the existing Role Admin? (this would require to add a validation)

Looking forward for your comments!
.

[julianmrodri]

@nventuro @frangio Would really appreciate your inputs so we can put this puppy to bed! Thanks for your time!

[nventuro]

Sorry for the delay @julianmrodri! We were a bit busy with the release of v3.0 and the Solidity Summity last week.

This looks great, thanks! I love how you followed our style for the docs and tests - I only left a small suggestion for further clarity.

Regarding only emitting on change, AccessControl is a bit special in that no-ops in grantRole and revokeRole don't emit events. This is to prevent confusion: RoleRevoked seems to imply the role had been previously granted, so we wanted to avoid revokes not preceeded by grants.

This case however seems more similar to an ERC20 transfer of 0. What if instead we added the previous role admin in the event? @frangio WDYT?

[frangio]

I agree we should include the current admin role in the event as well, similar to the OwnershipTransferred event. And I think we should always emit it, not only when it actually changes.

[julianmrodri]

Sounds good! Thanks! Will implement the change to add the current Admin Role and let you know.

[julianmrodri]

@nventuro @frangio I have implemented the changes you suggested.

• I added the "previousAdminRole" to the event. I followed the same naming you are using in OwnershipTransferred. I had to emit the event before updating the value now (the same is being done in 'OwnershipTransferred').
• Regarding the tests I had to break one statement into 3 lines due to lint issues (max length). TBH didn't know if use only 2 lines (leaving the first two parameters in the same line). I saw you were using one line per parameter in many cases so I followed that but we can change it if required.

Let me know what you think thanks for your support!

[nventuro]

Thank you very much @julianmrodri! I went ahead and adjusted the overly long line to a style that is more congruent with what we tend to use, and added an entry to the changelog for this improvement.

Hope to see more contributions from you soon! :)

[julianmrodri]

Thanks for your time and patience! for sure!