Skip to content
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.

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

Emit events before and after executing a message #577

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Conversation

alcuadrado
Copy link
Member

This is a WIP PR, as I'm not sure about

  1. Where to document this
  2. Where to test it. Should I create a new file in tests/api?

Closes #574

@s1na
Copy link
Contributor

s1na commented Aug 16, 2019

Where to document this

I just noticed we lack a comprehensive doc for the events. The API docs don't currently include the events. Should we maybe collect a list of the events in the README? Not sure where would be a good place for that.

Where to test it. Should I create a new file in tests/api?

Hm, the easiest would probably be in the GeneralStateTests as they already run txes with multiple messages and already do listen to some events, e.g.:

https://github.com/ethereumjs/ethereumjs-vm/blob/c3d7a57971a54b91294f37cd5f9b329733d89740/tests/GeneralStateTestsRunner.js#L78

but on the other hand, those tests are rather a black-box and we don't know the expected result for each message in advance

@holgerd77
Copy link
Member

Yes, events should really be documented (think we had some basic version before we switched to auto-generated docs, not sure?). Some are some techniques to do this with documentation, but maybe for now some manual addition to README will also do as @s1na suggested?

For tests I would find this more fitting as a new file in tests/api.

Update: did a quick search and found e.g. this on former event documentation, maybe we should simply do an adopted re-integration of the text in the README?

@holgerd77
Copy link
Member

Hi @alcuadrado, will you be able to add some docs and tests on this soon, so that we eventually can include in a subsequent v4.1.0 release (not too far away I think so that we incrementally get the Istanbul features out of the door and people can test)? Or shouldn't we wait for this and release independently?

@alcuadrado
Copy link
Member Author

I added tests for all of the events in this PR, and added documentation about them in the README.

@coveralls
Copy link

coveralls commented Sep 10, 2019

Coverage Status

Coverage increased (+0.1%) to 95.818% when pulling beb0892 on message-events into 7d9cc76 on master.

@alcuadrado
Copy link
Member Author

Oops, this was outdated wrt master. I rebased it.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! 😄

@holgerd77 holgerd77 merged commit a7532d6 into master Sep 10, 2019
@holgerd77 holgerd77 deleted the message-events branch September 10, 2019 21:45
@holgerd77
Copy link
Member

@s1na @alcuadrado I would now prepare release notes for a v4.1 release on Thursday.

@alcuadrado
Copy link
Member Author

Sounds good! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events before and after executing a Message
5 participants