Openzeppelin-contracts: Remove underscore from events' arguments

[elopio]

Some of our events have arguments with a leading underscore on their name. This underscore is unnecessary because there is not chance of ambiguity with these arguments, so it should be removed.

[elopio]

ERC721 defines the leading underscore on the events' arguments. If we change that, are we still following the standard?

[frangio]

Event argument names do not affect the ABI, so ERC721 contracts implemented without underscores will be perfectly compliant. Someone who uses the OpenZeppelin artifacts may expect underscores in the event arguments and will find that it won't work, but can easily adjust to the OpenZeppelin convention. I don't see a problem with it.

[maxsam4]

@frangio @elopio Sorry for bringing this back up but as far as I remember, Event names are included in the ABI (probably not in ByteCode though). IMO This change makes the tokens in openzeppelin non-compliant to ERC20 and ERC721.

Even if you think that it is still compliant, I hope you can still see the problems it will make for dApps trying to read ERC20 events. dApps won't know if the token is OZ or ERC20. The whole point of the standard was to have a standard way of doing things.

I don't see any reason except style consistency to remove the underscore. Maybe keep the underscore in the standard interfaces at least?

[maxsam4]

Can you guys please reopen this issue for further discussion among the community?
or create a new one and reference this and the concerned PR.

Thanks.

[frangio]

@maxsam4 Happy to continue the discussion here for now.

I don't see how this causes compatibility problems. Dapps aren't affected by the event argument names used in the source code, they just care about their order and indexedness, where OpenZeppelin is definitely compliant. This means that dapps don't need to know if the token is OZ or ERC20 as you implied.

Users of the OpenZeppelin interfaces will need to be aware of the lack of underscore prefixes. I think style consistency within the library is the more valuable thing from the perspective of those users, and we chose to prioritize that rather than doing verbatim what the ERC text says.

As a sidenote, it's not really clear what it means to be compliant with a Solidity interface defined in an ERC. Event argument names are only one of the issues. Perhaps we should write an ERC about it. :stuck_out_tongue:

[maxsam4]

Thanks for the response @frangio Here are my further thoughts

I don't see how this causes compatibility problems. Dapps aren't affected by the event argument names used in the source code, they just care about their order and indexedness, where OpenZeppelin is definitely compliant. This means that dapps don't need to know if the token is OZ or ERC20 as you implied.

I'll try to explain why some dApps will need to refactor their code to arguably worse code or will need to know if the token is OZ or ERC20.
When fetching events, Web3 1.X returns a returnValues object that maps event parameter name to value. Please refer to documentation and example available on https://web3js.readthedocs.io/en/1.0/web3-eth-contract.html#contract-events

When fetching events using Web3 0.X. it returns args object that does the same thing. Doc available on https://github.com/ethereum/wiki/wiki/JavaScript-API
A sample code:

contract.SampleContract ().watch ( (err, response) => {  
    console.log(response.args.eventParameterName);
});

I know we can use topics array for the same purpose but it has disadvantages:
1) You have to remember array index rather than name. Or check from code every time. Extra hassle for no reason.
2) You have to refactor your code.
3) Code's readability slightly decreases.

Kinda similar to why we give proper names to variables :) You don't have to but it makes life easier.

I think style consistency within the library is the more valuable thing from the perspective of those users, and we chose to prioritize that rather than doing verbatim what the ERC text says.

I'd have to disagree here. If a contract is not ERC20 verbatim, I will not call it ERC20 complaint. In my opinion, You can freely add stuff that the standard does not prohibit like reverting rather than returning false but you should not change what the standard defines as "SHOULD" or "MUST".

As a sidenote, it's not really clear what it means to be compliant with a Solidity interface defined in an ERC. Event argument names are only one of the issues. Perhaps we should write an ERC about it. stuck_out_tongue

I agree with this one. Everyone has their opinion. An ERC might actually help :stuck_out_tongue:

[frangio]

I agree that using indexes instead of names is a huge hassle and compromise to readability. We would never encourage our users to do that.

When fetching events, Web3 1.X returns a returnValues object that maps event parameter name to value.

The object will use the parameter names as declared by the JSON interface used in the client, not in the source code for a contract address (which are unknown as far as the EVM goes).

Removing the underscore prefixes in OpenZeppelin allows clients built with our JSON interfaces to use non-prefixed parameter names, but it does not affect at all the ERC20 compliance of the produced bytecode.

[maxsam4]

I agree that using indexes instead of names is a huge hassle and compromise to readability. We would never encourage our users to do that.

Thank you for understanding.

The object will use the parameter names as declared by the JSON interface used in the client, not in the source code for a contract address (which are unknown as far as the EVM goes).

I don't want to ship pre-defined ABIs with my dApp. I want to ship the contract source code as a module and have them compiled by the user. Similar to how open zeppelin does not ship Bytecode and ABIs.
This change hurts more when we already have deployed stuff to the main net. We'll have to use a different ABI than what the original source code produces. For example, our utility token has a classic code that generates ABI with underscores while newer tokens being generated from our beta dApp won't have the underscore in the ABI. Either we'll be forced to use a slightly modified ABI for one of these or use indexes instead of names.

Removing the underscore prefixes in OpenZeppelin allows clients built with our JSON interfaces to use non-prefixed parameter names, but it does not affect at all the ERC20 compliance of the produced bytecode.

IMHO this change to the interface was not required. Style consistency is meant to make interpreting stuff easier rather than creating more confusion.
I consider ABI to be part of the ERC. Given that standards usually define the interface rather implementation, I'd argue that ABI is just as important if not more than bytecode for compliance. But it's my own interpretation and I can see why others might consider only the Bytecode to be compliant with an ERC.

[elopio]

@maxsam4 for your code already deployed to mainnet, shouldn't you use the OpenZeppelin version you used when you deployed, instead of the latest?

[maxsam4]

@maxsam4 for your code already deployed to mainnet, shouldn't you use the OpenZeppelin version you used when you deployed, instead of the latest?

Our token on Mainnet does not use OpenZeppelin actually.
The security tokens generated through our platform use OpenZeppelin and the version is pinned.
The problem is that we are changing to OZ 2.0 for the next version of our security tokens.

I want both their ABIs to have same event parameter names so that I don't have to treat them differently when treating them as ERC20. I will still be loading both their ABIs separately though as other non ERC20 functions differ.

It's not that I can't refactor my code. It is rather straightforward to make the required changes than trying to change OpenZeppelin :).

The bigger concern to me is that changing a standard's interface changes the ABI and it makes the token non-compliant (IMO). The above is just an example of the inconvenience it causes.

[elopio]

Got it. Then I agree with @frangio that we need an ERC that defines compliance. And I'll add that we need another one with a suggested style for all ERCs 😃