Convention around indexed string arguments in events (RBAC)

[frangio]

The RBAC events currently do not have indexed arguments.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/b0292cf628d9f8d65615bc07dd8af4844eb5a6d6/contracts/ownership/rbac/RBAC.sol#L22-L23

In #981 @shrugs made the address argument indexed, and it made me think that string roleName should be indexed as well to efficiently look up all the activity around a specific role. The problem is that Solidity will not include the actual contents of an indexed string in the log data, but the hash of the string. This shouldn't stop us.

I thought of two alternative (complementary?) workarounds for this issue, which basically enable clients to get a human-readable representation of the hash without having to do a preimage attack on KECCAK256.

1. At deployment emit an event RoleRegistered(string indexed role, string roleName).
2. Provide an external getter function roleName(bytes32 role) returns (string).

Personally I'm inclined for doing only 1, so as to not clutter the external inteface, but I would like to back that decision up with data on how efficient it would be for a client to get the role name given its hash emitted in one of the other events.

Something like this should probably be the convention for all Solidity programs that wish to use indexed strings in events. What do you think?

[shrugs]

I like the RoleRegistered approach, so I can just build an off-chain map of hash->human roles. imo the mapping of hash->human doesn't need to live on-chain.

It might be worth standardizing this as something like WellKnownHashRegistered(string indexed hash, string humanReadable) and then we can build an off-chain indexer (like with gnarly!) that just knows to look for that event on any contract it cares about, and can then auto-infer hashes to human readable forms.

But that could also be more effort than it's worth for now? Idk, I'm ok with making this an EIP proposal and then immediately applying it to OZ contracts.

[nventuro]

RBAC no longer exists and Roles have dedicated events, so this doesn't apply anymore.