Skip to content

feat: make proposalId indexed in ProposalCreated event #3826 #3827

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

Closed
wants to merge 1 commit into from
Closed

feat: make proposalId indexed in ProposalCreated event #3826 #3827

wants to merge 1 commit into from

Conversation

paknecht
Copy link

@paknecht paknecht commented Nov 19, 2022

Fixes #3826

make proposalId indexed in ProposalCreated event

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@paknecht
Copy link
Author

Question 1: Should I also make a PR in this repo: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable or is that transferred somehow automatically?
Question 2: Do I need to change anything in the documentation? If so, where exactly? Have found nothing in a quick search

@Amxx
Copy link
Collaborator

Amxx commented Nov 19, 2022

Hello @paknecht

For the record, the event is that way for compatibility with GovernorBravo (which predates our implementation).

Changing that, while it would not change the event's "topic0", would break a lot of indexing systems. So yes, it would be nice to index that, but it would also be a breaking change :/

@paknecht
Copy link
Author

Hi @Amxx
Thanks for looking at this PR.
I'm new in solidity but I would assume that uint to uint256 is already a breaking change and therefore the GovernorBravo Event isn't compatible to Governor?

Who can decide if this breaking change here is acceptable? Or can't we make breaking changes in general?

@Amxx
Copy link
Collaborator

Amxx commented Nov 19, 2022

uint is just an alias to uint256. They are the same thing, and using one over the other has no impact on event topic or function selector.

We are currently fully compatible with GovernorBravo events.

@Amxx
Copy link
Collaborator

Amxx commented Nov 19, 2022

Whenever we consider making a breaking change, we try to weight in the advantages and the drawback. We sometimes do them, but we need a very strong reason.

For example, fixing a severe bug fully justify a breaking change.

In your case, the drawback is to break some indexing services. The governor subgraphs are one thing, but other services like Tally would also be affected. The upside of having this parameter indexed exists but its not that clear to me.

@frangio
Copy link
Contributor

frangio commented Jan 4, 2023

Closing to move discussion to the issue: #3826

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.

Indexed proposalId in Governor.ProposalCreated event
3 participants