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

improve: add ProposalExecuted event #4374

Closed
wants to merge 3 commits into from
Closed

Conversation

pemulis
Copy link
Contributor

@pemulis pemulis commented Jan 10, 2023

Signed-off-by: John Shutt pemulis@users.noreply.github.com

Motivation

We want a ProposalExecuted event tied to the original proposal time to make it easier for the Snapshot interface to determine whether a specific proposal was executed, and differentiate between duplicate transaction bundles that were created at different times.

Summary

Adds a ProposalExecuted event that includes the proposal hash and the proposal time, which triggers on completion of the executeProposal function.

Details

For the V2 version of the contract, we should consider adding proposalTime to other events like TransactionExecuted and ProposalDeleted. To keep this PR limited, I did not alter those events.

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

Issue(s)

Fixes #XXXX

Signed-off-by: John Shutt <pemulis@users.noreply.github.com>
Signed-off-by: John Shutt <pemulis@users.noreply.github.com>
@pemulis pemulis marked this pull request as ready for review January 10, 2023 20:47
@pemulis pemulis requested a review from abg4 January 10, 2023 20:48
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

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

LGTM. let's get this in.

@chrismaree
Copy link
Member

This is no longer relevent.

@chrismaree chrismaree closed this Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants