Add candidate events #2754
Add candidate events #2754
Conversation
| }, | ||
| new ContractEventDescriptor | ||
| { | ||
| Name = "CandidateUnregistered", |
There was a problem hiding this comment.
Merge it with CandidateRegistered? Maybe CandidateStateChanged or something.
There was a problem hiding this comment.
Explicit events seems to match closer to existing events in the system. For example; we have Update, Destroy and Deploy instead of ContractStateChanged with a New/Updated/Destroyed parameter.
There was a problem hiding this comment.
If we have Register and Unregister, then we need Vote and Unvote.
There was a problem hiding this comment.
I'd be totally fine with that too. However, we do not have Burn or Mint either. We just have Transfer with a Null to or from respectively. It's not like everything needs to be (or even is) symmetrical in the system.
There was a problem hiding this comment.
Then I prefer to merge Register and Unregister.🤭
There was a problem hiding this comment.
Unifying just makes post processing "harder" where explicit you can switch on the event name.
Then I prefer to merge Register and Unregister.🤭
I was trying to point out that vote could work similar to the transfer event. How about we vote for it
🎉 for all explicit (CandidateRegistered, CandidateUnregistered, Vote, Unvote)
❤️ for implicit (CandidateStateChanged with extra param for Registered/Unregistered state, vote with no extra parameter and the consumer parses the to field to determine if it is a vote or unvote)
There was a problem hiding this comment.
I'd definitely like to see one Vote event. But for some reason I also like two events for candidate registration/unregistration. Go figure. Maybe that's because we have one vote method and a pair of registerCandidate/unregisterCandidate. At the same time a pair of methods doesn't mean we need to have a pair of events.
There was a problem hiding this comment.
My first choice would have been registerCandidate/unregisterCandidate and a single vote, pretty much how it was PR'ed. But Erik seems to want all explicit or all implicit hence the 2 choices above 🤷♂️
|
Just as a side note --- how about committee change events? Committee is a very powerful entity in the system and monitoring changes there can be important. We can emit notifications only when something changes there to optimize the number of events. |
If we decide to add it, it's better to put it in a separate pr. |
Co-authored-by: Erik Zhang <erik@neo.org>
Close #2752