Skip to content

Adding event and error selector fields #12921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
chriseth merged 1 commit into from
May 23, 2022
Merged

Adding event and error selector fields #12921

chriseth merged 1 commit into from
May 23, 2022

Conversation

@nishant-sachdeva
Copy link
Contributor

@nishant-sachdeva nishant-sachdeva commented Apr 11, 2022

closes #12581

@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 4 times, most recently from 6dccdd2 to 22a7a5c Compare April 14, 2022 14:13
@nishant-sachdeva nishant-sachdeva dismissed a stale review via a35d71c April 18, 2022 12:30
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 3 times, most recently from 8b0b96d to 562ed07 Compare April 18, 2022 12:32
@nishant-sachdeva nishant-sachdeva marked this pull request as ready for review April 18, 2022 12:36
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 6 times, most recently from ed6c4e9 to 480595b Compare April 19, 2022 14:00
@stackenbotten
Copy link

stackenbotten commented Apr 19, 2022

There was an error when running chk_coding_style for commit 480595b4fd37c04c6610f961154c783bbe9994fd:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/error/error_selector.sol:23:

Please check that your changes are working as intended.

@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 4 times, most recently from d5c567f to fa09d90 Compare April 19, 2022 16:47
hrkrshnn
hrkrshnn reviewed Apr 19, 2022
View reviewed changes
hrkrshnn
hrkrshnn reviewed Apr 19, 2022
View reviewed changes
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 8 times, most recently from 977f545 to 472a16b Compare April 25, 2022 04:47
ekpyron
ekpyron reviewed May 13, 2022
View reviewed changes
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch from 59de325 to 25ec156 Compare May 13, 2022 09:46
ekpyron
ekpyron reviewed May 13, 2022
View reviewed changes
ekpyron
ekpyron reviewed May 13, 2022
View reviewed changes
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 4 times, most recently from ccdc55c to a3d7bac Compare May 13, 2022 17:53
hrkrshnn
hrkrshnn reviewed May 13, 2022
View reviewed changes
...ty/syntaxTests/events/event_selector_library_declared_outside_but_called_inside_function.sol
}
}
// ----
// TypeError 9582: (70-82): Member "selector" not found or not visible after argument-dependent lookup in function ().
Copy link
Contributor

@hrkrshnn hrkrshnn May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function ().

Nitpick: need to fix the error message. I think there is a PR already for this.

Copy link
Contributor

@hrkrshnn hrkrshnn May 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#13005 is the PR

@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch from a3d7bac to 43dff44 Compare May 17, 2022 12:42
@StrongerXi StrongerXi mentioned this pull request May 17, 2022
Merged
@nishant-sachdeva nishant-sachdeva force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch 5 times, most recently from 6f3e7ba to c7bd783 Compare May 20, 2022 15:56
ekpyron
ekpyron reviewed May 20, 2022
View reviewed changes
docs/units-and-global-variables.rst

- ``string.concat(...) returns (string memory)``: :ref:`Concatenates variable number of string arguments to one string array<string-concat>`


Copy link
Collaborator

@ekpyron ekpyron May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I guess we could remove the newline here, it'd be nicer to keep the PR clean of unrelated changes like this :-).

ekpyron
ekpyron previously approved these changes May 20, 2022
View reviewed changes
Copy link
Collaborator

@ekpyron ekpyron left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. If we merge we should remember rebasing #13005 afterwards, though, since it may change the error messages of the new tests.

chriseth
chriseth reviewed May 23, 2022
View reviewed changes
test/libsolidity/syntaxTests/events/event_selector_library.sol
@@ -0,0 +1,17 @@
library Y {
Copy link
Contributor

@chriseth chriseth May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of the test should include the word 'anonymous' since this is what is being tested.

Please also add more tests for different ways to access the selector of an anonymous event.

docs/contracts/events.rst Outdated
Members of Events
=================

- ``event.selector``: The signature is defined as the canonical expression of the basic prototype without data location specifier. It is the the Keccak-256 hash of the signature of the event.
Copy link
Contributor

@chriseth chriseth May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change this before merging.

@chriseth chriseth dismissed stale reviews from ghost and ekpyron via d4c06d2 May 23, 2022 08:49
@chriseth chriseth force-pushed the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch from c7bd783 to d4c06d2 Compare May 23, 2022 08:49
@chriseth chriseth enabled auto-merge May 23, 2022 08:50
@chriseth chriseth merged commit f0e4392 into develop May 23, 2022
@chriseth chriseth deleted the add_event_and_error_selector_fields_on_the_line_of_function_selector_field branch May 23, 2022 09:34
@cameel cameel mentioned this pull request Jun 1, 2022
Closed
This was referenced Jun 14, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekpyron ekpyron ekpyron left review comments

+4 more reviewers

@hrkrshnn hrkrshnn hrkrshnn left review comments

@wechman wechman wechman left review comments

@stackenbotten stackenbotten stackenbotten left review comments

@chriseth chriseth chriseth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extend the language to have an EVENT.selector and ERROR.selector

7 participants

@nishant-sachdeva @stackenbotten @chriseth @ekpyron @hrkrshnn @wechman