NatSpec and multiple events with identical signature

[chriseth]

trafficstars

It is impossible to define multiple events with the same signature in the same contract. This lead to the design that the user documentation ("NatSpec") has a mapping of event signatures to the description of the event.

What has been possible for a while now is for a contract to emit events with the same signature but different natspec through the use of libraries or via referencing events of a different contract.. The events emitted this way were ignored in the natspec output so far.

We now have different options to solve this situation:

• change the natspec json to use arrays instead of direct objects
• only use "the first" non-empty natspec (prioritize the event in the current contract or a base and then use AST IDs for priorization)
• somehow combine the documentations (difficult for parameters etc)
• issue a warning if such a clash happens

[hrkrshnn]

I want to add that currently event E(uint indexed) and event E(uint) have the same signature. (Both can't occur in the same contract currently.)

Maybe we should really add "indexed" to the signature.

[axic]

@hrkrshnn there's a conversation about that in #4168 -- in short, breaking ABI change. There are some other issues I can't find now.

[hrkrshnn]

To summarize yesterday's lengthy discussion. Feel free to edit this if I missed something:

• Discussion on a solution about NatSpec when events with same signature are defined in different scopes.
  • Only relevant after the new callgraph for Events that can detect events emitted inside libraries and free functions.
• Some initial agreement about changing the 'value' field for event signature in JSON to be an array instead of object.
• @axic: better to include other NatSpec changes along with this change.
• @chriseth mentioned that the situation is identical for errors.
  • Although, towards the end, mentioned that currently errors will not have signature collisions, so making it an array is not necessary.
• Question: Should we always change the 'value' field for Events and Errors to be an array, or only when there are repetitions?
  • Opinion (@cameel, @hrkrshnn, @axic(?)) it's better to keep it as an array always for consistency.
  • @chriseth mentioned to implement Error NatSpec as arrays right away. Change of opinion towards the end (see point above.)
• @axic mentioned that the NatSpec version field can be updated if we change object into array.
  • Discussed whether this has to be done on Solidity breaking as well. Agreement on how bumping NatSpec version is sufficient.
• @aarlt: we should not allow Events with same signature. Convert them into a warning or an error at the contract.
  • No clear agreement about what should be done for Events after this.
• Asking about a solution that can be implemented this week.
  • @chriseth mentioned that he'll proceed with just Objects for Errors, which is what's currently implemented. And decide about events later.

[chriseth]

It turns out that the situation is not different for errors.

Since nobody is using natspec, I tend to increment the version counter for the next release and collect some of the changes we plan to do.