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

Fix pending root event emission #80

Closed

Conversation

QGarchery
Copy link
Contributor

Fixes:

Also unifies two events that were about the pending root update. Now the pending root value are always equal to the values in the last PendingRootSet event

@QGarchery QGarchery requested review from MathisGD and a team October 27, 2023 16:56
@QGarchery QGarchery self-assigned this Oct 27, 2023
@QGarchery QGarchery requested review from Rubilmax, MerlinEgalite and Jean-Grimal and removed request for a team October 27, 2023 16:56
@QGarchery QGarchery changed the base branch from main to fix/review-simple-refactors October 27, 2023 16:57
@QGarchery
Copy link
Contributor Author

Why was this run canceled ?

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

I think it makes less clear when a revoke is invoked. @julien-devatom wdyt?

@julien-devatom
Copy link
Contributor

I think it makes less clear when a revoke is invoked. @julien-devatom wdyt?

Yes exactly, you miss the information that the pending value is revoked. So for this reason, I'm against this refactoring.

@QGarchery
Copy link
Contributor Author

Yes exactly, you miss the information that the pending value is revoked

This information is ambiguous though, because trigerring _setRoot effectively deletes the pending root. So, for example, if you use the number of RevokeRoots to know how many proposals of roots have been revoked you are counting it wrong. It could be that the owner revokes the roots by using the setRoot function with the same root and the same ipfsHash.

What is important is to be able to track the changes of the values of the pending root. Adding an event for the special case of the bytes32(0) just makes it more difficult (because now you have to handle another event).

Also, don't miss the fact that this PR also fixes the fact that no events was emitted for the pending root being deleted in _setRoot

@MerlinEgalite
Copy link
Contributor

This information is ambiguous though, because trigerring _setRoot effectively deletes the pending root.

This is a good point actually but the opposite is true as well? How do you know when someone revoke a root or that the root has been consumed (everyone has claimed their rewards) and then reset to 0

@QGarchery
Copy link
Contributor Author

QGarchery commented Oct 31, 2023

This is a good point actually but the opposite is true as well? How do you know when someone revoke a root or that the root has been consumed (everyone has claimed their rewards) and then reset to 0

Yes it's true, you can't tell them apart. And you also can't tell if everyone has claimed their reward, because that information is not available (it's in the hash). So my point is that we should not try to add events that make it look like this information can be retrieved

@MerlinEgalite
Copy link
Contributor

Should we keep that PR open @QGarchery @julien-devatom ?

@julien-devatom
Copy link
Contributor

I'm closing it since this is discussed & handled in the PR #83

@MerlinEgalite MerlinEgalite deleted the fix/pending-root-event branch November 6, 2023 09:09
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.

[protocol review] Events are not reliably emitted for the pending root updates
3 participants