Export all events #10996

chriseth · 2021-02-23T13:18:45Z

Fixes #9765.
Fixes #13086.

Changelog.md

libsolidity/ast/AST.h

libsolidity/ast/ASTJsonConverter.cpp

chriseth · 2021-02-25T15:10:00Z

Updated.

hrkrshnn · 2021-02-25T13:06:28Z

libsolidity/ast/AST.cpp

+		solAssert(annotation().creationCallGraph.set() == annotation().deployedCallGraph.set(), "");
+		if (annotation().creationCallGraph.set())
+		{
+			unfilteredEvents += (*annotation().creationCallGraph)->emittedEvents;


Not entirely sure if events emitted during creation should be part of this. Is it something that contracts actually do?

Why shouldn't they? My intention was to include all events that can originate from this address (and which are not caused by a delegatecall).

libsolidity/ast/AST.cpp

libsolidity/ast/ASTJsonConverter.cpp

hrkrshnn · 2021-02-25T16:05:26Z

libsolidity/ast/AST.cpp

+			solAssert(function, "");
+			if (!function->interfaceFunctionType() && _ignoreErrors)
+				continue;
+			string eventSignature = function->externalSignature();


I think this would cause issues when in the following case:

library L { event e1(uint b); } function f() { emit L.e1(5); } contract C { event e1(uint indexed a); function g() public { f(); } }

Because the signature does not contain "indexed".

The ABI for this test case is:

[ { "anonymous": false, "inputs": [ { "indexed": false, "internalType": "uint256", "name": "a", "type": "uint256" } ], "name": "e1", "type": "event" }, { "inputs": [], "name": "g", "outputs": [], "stateMutability": "nonpayable", "type": "function" } ]

Which doesn't contain the library event.

We have the same problem during inheritance. Let me see what I can do.

I actually don't know why we do this filtering at all. Events can be overloaded but defining two events with the same name and parameter types in an inheritance hierarchy is an error anyway.

chriseth · 2021-02-25T16:24:41Z

We ignore errors in #10996 (comment) because the ast can be exported for not fully valid source code.

test/libsolidity/ASTJSON/fallback_and_reveice_ether.json

chriseth · 2021-03-01T12:59:40Z

Updated.

libsolidity/ast/ASTJsonConverter.cpp

libsolidity/ast/AST.cpp

hrkrshnn · 2021-03-09T16:19:12Z

libsolidity/ast/AST.cpp

-		set<string> eventsSeen;
-		vector<EventDefinition const*> interfaceEvents;
-
+		std::set<EventDefinition const*, ASTNode::CompareByID> interfaceEvents;


In this example,

library L1 { event e(); } library L2 { event e(); } contract C { function f() public { emit L1.e(); emit L2.e(); } }

The event e would be repeated twice. Technically, only one is enough. Maybe repetition in the abi would be an issue for front ends. Another example would be a contract having the same event as a library.

If you want, we can probably merge this already and potentially fix this issue later.

It is two different events. I think it's good to list multiple events. We could filter, but hm.

Aren't they the same log from EVM's perspective? There is nothing distinguishing about them, is there?

Yeah, maybe you are right. But I would propose to filter them at a different place.

I'll filter them if the json is exactly the same. If there is a difference e.g. in parameter names, then both will stay.

libsolidity/interface/ABI.cpp

test/libsolidity/ABIJson/same_event_defined_twice.sol

hrkrshnn · 2021-03-15T10:08:38Z

About the NatSpec issue:

Concatenating the NatSpec of events with same signature sounds good.

There is this edge case, which should be two different events in the NatSpec, but ends up being the same since we omit the "indexed" part from signature.

library L {
    /// library
    event E(uint indexed a);
}
contract  C {
    constructor() {
        emit L.E(1);
    }
    /// contract
    event E(uint a);
}

hrkrshnn · 2021-03-15T10:21:50Z

Also, web3.py seems to throw an error when accessing an event with name E if there are multiple events with name E in the ABI. Makes me wonder if we should disallow events with the same name, even though at EVM level they make sense.

chriseth · 2021-03-16T16:16:45Z

Actually concatenating natspec does not really work. Do we concatenate the values of all the different fields? What if the parameters have different names?

cameel

Ok, now it looks good!

I have a small simplification suggestion for event processing, but other than that, I think we're finally done here.

libsolidity/interface/Natspec.cpp

cameel · 2023-04-28T23:08:52Z

Please squash review fixes into the commits.

matheusaaguiar · 2023-04-29T01:59:09Z

There are these two pending questions/comments:

Export all events #10996 (comment) - comment about filtering events not part of the external interface
Export all events #10996 (comment) - add asserts (not a file-level event, an event from another contract)

The first one, I didn't quite understand, I guess it is about events emitted in private/internal functions ?
The second one I am working on it.

cameel · 2023-04-29T02:46:31Z

We'd need to ask @chriseth about the comment, I think he's the one who wrote it. Would be best to find out, because it's confusing, but it's not serious enough to block the PR.

Asserts are also just nice to have. They shouldn't be anything elaborate. Only things you can check fairly easily without writing lots of support code that's not used for anything else.

cameel

The extra tweaks we discussed would be nice but I'm also already approving since they're not important enough to block merging the PR.

matheusaaguiar · 2023-04-29T23:21:17Z

The extra tweaks we discussed would be nice but I'm also already approving since they're not important enough to block merging the PR.

Ok. I just added the asserts in a different commit so they can be removed if we want to merge the PR without them.
Also, I thought that events could not be emitted from inherited interfaces. I was not sure so I added some tests about that.

Changelog.md

ekpyron · 2023-05-03T14:31:30Z

test/libsolidity/SolidityNatspecJSON.cpp

@@ -485,6 +485,411 @@ BOOST_AUTO_TEST_CASE(event)
 	checkNatspec(sourceCode, "ERC20", userDoc, true);
 }

+BOOST_AUTO_TEST_CASE(emit_same_signature_event_library_contract)


Just for future reference: this is beyond the amount of hand-crafted cpp boost test cases, s.t. I'd have transitioned this stuff to nicer isoltest tests. But since we have them now like this, it's fine and we can keep them here. But yeah: general policy: more than one test of a few lines like this we should already think about adding a new isoltest test kind - the infrastructure for adding simple ones there is rather convenient these days.

libsolidity/interface/Natspec.cpp

Update tests. Additional tests Revert changes to the Natspec

matheusaaguiar · 2023-05-03T17:10:03Z

Rebased and squashed commits.

ekpyron · 2023-05-03T17:19:46Z

Changelog.md

 * Antlr Grammar: Fix discrepancy with the parser, which allowed octal numbers.
 * Antlr Grammar: Fix of a discrepancy with the parser, which allowed numbers followed by an identifier with no whitespace.
 * Antlr Grammar: Stricter rules for function definitions. The grammar will no longer accept as valid free functions having specifiers which are exclusive to contract functions.
 * SMTChecker: Fix false positives in ternary operators that contain verification targets in its branches, directly or indirectly.


 AST Changes:
- * AST: add the ``internalFunctionID`` field to the AST nodes of functions that may be called via the internal dispatch. These IDs are always generated, but they are only used in via-IR code generation.
+ * AST: Add the ``internalFunctionID`` field to the AST nodes of functions that may be called via the internal dispatch. These IDs are always generated, but they are only used in via-IR code generation.


Why is this line showing up in the diff by the way? Not sure if I'm blind, but I don't see any change in it...

Capitalized A in Add at the beginning of the sentence... had to look a dozen of times to spot it :)

Haha, lol - but that's also a bug in github - it usually highlights the actual change, here it doesn't :-)

ekpyron

Approving despite https://github.com/ethereum/solidity/pull/10996/files?diff=split&w=1#r1183995391 seeming a bit odd.

Dismissing old outdated review.

chriseth requested review from aarlt, axic, hrkrshnn and mijovic February 23, 2021 13:19

chriseth force-pushed the exportAllEvents branch 3 times, most recently from 63273d5 to 4eb4c5e Compare February 23, 2021 16:46

axic reviewed Feb 24, 2021

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

axic reviewed Feb 24, 2021

View reviewed changes

libsolidity/ast/AST.h Outdated Show resolved Hide resolved

axic reviewed Feb 24, 2021

View reviewed changes

libsolidity/ast/ASTJsonConverter.cpp Outdated Show resolved Hide resolved

chriseth force-pushed the exportAllEvents branch from 4eb4c5e to cc879be Compare February 25, 2021 15:09

hrkrshnn requested changes Feb 25, 2021

View reviewed changes

cameel reviewed Feb 25, 2021

View reviewed changes

test/libsolidity/ASTJSON/fallback_and_reveice_ether.json Outdated Show resolved Hide resolved

chriseth force-pushed the exportAllEvents branch 2 times, most recently from 7e97630 to ddff575 Compare March 3, 2021 14:47

hrkrshnn previously requested changes Mar 3, 2021

View reviewed changes

libsolidity/ast/ASTJsonConverter.cpp Outdated Show resolved Hide resolved

libsolidity/ast/AST.cpp Outdated Show resolved Hide resolved

chriseth force-pushed the exportAllEvents branch 4 times, most recently from 4eadaf0 to f9ebbff Compare March 9, 2021 16:06

hrkrshnn reviewed Mar 9, 2021

View reviewed changes

chriseth force-pushed the exportAllEvents branch from f9ebbff to 204fd4e Compare March 9, 2021 17:03

chriseth commented Mar 9, 2021

View reviewed changes

libsolidity/interface/ABI.cpp Outdated Show resolved Hide resolved

chriseth commented Mar 9, 2021

View reviewed changes

test/libsolidity/ABIJson/same_event_defined_twice.sol Show resolved Hide resolved

cameel previously approved these changes Apr 28, 2023

View reviewed changes

libsolidity/interface/Natspec.cpp Outdated Show resolved Hide resolved

matheusaaguiar dismissed cameel’s stale review via d0bf624 April 29, 2023 01:24

matheusaaguiar force-pushed the exportAllEvents branch from d0bf624 to 70e6064 Compare April 29, 2023 01:50

cameel previously approved these changes Apr 29, 2023

View reviewed changes

matheusaaguiar dismissed cameel’s stale review via c508d7f April 29, 2023 23:12

matheusaaguiar force-pushed the exportAllEvents branch from 70e6064 to c508d7f Compare April 29, 2023 23:12

matheusaaguiar force-pushed the exportAllEvents branch from c508d7f to 71af715 Compare April 30, 2023 02:08

Amxx mentioned this pull request May 1, 2023

Turn ERC1967Upgrade into a library OpenZeppelin/openzeppelin-contracts#4204

Closed

ekpyron reviewed May 2, 2023

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

matheusaaguiar force-pushed the exportAllEvents branch from 2ee96d3 to 910a136 Compare May 3, 2023 14:04

ekpyron reviewed May 3, 2023

View reviewed changes

libsolidity/interface/Natspec.cpp Outdated Show resolved Hide resolved

Export all events.

1e63615

Update tests. Additional tests Revert changes to the Natspec

matheusaaguiar force-pushed the exportAllEvents branch from 910a136 to 1e63615 Compare May 3, 2023 17:09

ekpyron reviewed May 3, 2023

View reviewed changes

ekpyron approved these changes May 3, 2023

View reviewed changes

ekpyron merged commit dde579b into develop May 3, 2023

ekpyron deleted the exportAllEvents branch May 3, 2023 19:32

nikola-matic mentioned this pull request May 5, 2023

Introduce pragma experimental solidity #14168

Merged

4 tasks

Amxx mentioned this pull request May 10, 2023

Emit event declared in (non inherited) interfaces #14206

Closed

cameel mentioned this pull request May 26, 2023

Allow qualified access to events from other contracts #14274

Merged

This was referenced Jul 20, 2023

Testing Natspec JSON via isoltest #14433

Closed

Fix ICE when emitting event from another contract #14434

Merged

cameel mentioned this pull request Sep 15, 2023

Duplicated error definitions in the ABI #14404

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export all events #10996

Export all events #10996

chriseth commented Feb 23, 2021 •

edited by cameel

Loading

chriseth commented Feb 25, 2021

hrkrshnn Feb 25, 2021

chriseth Feb 25, 2021

hrkrshnn Feb 25, 2021

chriseth Feb 25, 2021

chriseth Mar 1, 2021

chriseth commented Feb 25, 2021

chriseth commented Mar 1, 2021

hrkrshnn Mar 9, 2021

chriseth Mar 9, 2021

hrkrshnn Mar 9, 2021 •

edited

Loading

chriseth Mar 9, 2021

chriseth Mar 9, 2021

hrkrshnn commented Mar 15, 2021

hrkrshnn commented Mar 15, 2021

chriseth commented Mar 16, 2021

cameel left a comment

cameel commented Apr 28, 2023

matheusaaguiar commented Apr 29, 2023

cameel commented Apr 29, 2023 •

edited

Loading

cameel left a comment

matheusaaguiar commented Apr 29, 2023 •

edited

Loading

ekpyron May 3, 2023

matheusaaguiar commented May 3, 2023

ekpyron May 3, 2023

matheusaaguiar May 3, 2023 •

edited

Loading

ekpyron May 3, 2023

ekpyron left a comment

Export all events #10996

Export all events #10996

Conversation

chriseth commented Feb 23, 2021 • edited by cameel Loading

chriseth commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Feb 25, 2021

chriseth commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn commented Mar 15, 2021

hrkrshnn commented Mar 15, 2021

chriseth commented Mar 16, 2021

cameel left a comment

Choose a reason for hiding this comment

cameel commented Apr 28, 2023

matheusaaguiar commented Apr 29, 2023

cameel commented Apr 29, 2023 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

matheusaaguiar commented Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

matheusaaguiar commented May 3, 2023

Choose a reason for hiding this comment

matheusaaguiar May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron left a comment

Choose a reason for hiding this comment

chriseth commented Feb 23, 2021 •

edited by cameel

Loading

hrkrshnn Mar 9, 2021 •

edited

Loading

cameel commented Apr 29, 2023 •

edited

Loading

matheusaaguiar commented Apr 29, 2023 •

edited

Loading

matheusaaguiar May 3, 2023 •

edited

Loading