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.

Sign up for GitHub

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

Jump to bottom

Rename RootRevoked to PendingRootRevoked #83

Merged
merged 15 commits into from
Nov 7, 2023
Merged

Rename RootRevoked to PendingRootRevoked #83

merged 15 commits into from
Nov 7, 2023

Conversation

julien-devatom
Copy link
Contributor

@julien-devatom julien-devatom commented Oct 28, 2023 •
edited by MerlinEgalite
Loading

@julien-devatom julien-devatom self-assigned this Oct 28, 2023
@julien-devatom julien-devatom requested review from a team, Rubilmax, MerlinEgalite, Jean-Grimal, QGarchery, peyha and MathisGD and removed request for a team October 30, 2023 10:47
QGarchery
QGarchery requested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@QGarchery QGarchery left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Having an event just for the root being revoked is counter-productive, because when you are tracking the pending root you prefer having only one event. I think that reusing the same event, but with bytes32(0) as arguments would be best, as it is done in #80 (more details in this comment)

@Rubilmax
Copy link
Contributor

In any case it must be consistent with MetaMorpho

@julien-devatom
Copy link
Contributor Author

Waiting for a consensus

@MerlinEgalite
Copy link
Contributor

Again please set the issue that this PR is tackling in the header please (I did it)

MerlinEgalite
MerlinEgalite requested changes Oct 31, 2023
View reviewed changes
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.

Let's apply @QGarchery suggestion and apply it to MM as well

@julien-devatom julien-devatom changed the title refactor(urd): revokeRoot => revokePendingRoot remove RevokePendingRoot event and use SetPendingRoot instead Oct 31, 2023
@julien-devatom julien-devatom changed the title remove RevokePendingRoot event and use SetPendingRoot instead remove RevokePendingRoot event and use RootProposed instead Oct 31, 2023
@julien-devatom julien-devatom changed the title remove RevokePendingRoot event and use RootProposed instead remove RevokePendingRoot event and use PendingRootSet instead Oct 31, 2023
@julien-devatom
Copy link
Contributor Author

julien-devatom commented Nov 2, 2023 •
edited
Loading

After a discussion with @QGarchery & @Rubilmax, we have decided to keep the PendingRootRevoked event in case of a call to revokePendingRoot, because if we emit the event PendingRootSet(0,0), we cannot distinguish the submission of a 0 root and the case of a revoke call.

Moreover, the function revokePendingRoot can be called by all updaters since they can always override the pending root by submitting a new value.

So, to recap the final state:

  • PendingRootSet is emitted each time a new pending root value is submitted
  • RootSet is emitted if the owner bypasses the timelock or if the pending value is accepted
  • PendingRootRevoked is emitted if an updater revokes a pending value.

Rubilmax
Rubilmax approved these changes Nov 2, 2023
View reviewed changes
src/UniversalRewardsDistributor.sol Outdated Show resolved Hide resolved
src/libraries/EventsLib.sol Outdated Show resolved Hide resolved
src/UniversalRewardsDistributor.sol Show resolved Hide resolved
@Rubilmax Rubilmax changed the title remove RevokePendingRoot event and use PendingRootSet instead Rename RootRevoked to PendingRootRevoked Nov 2, 2023
@Rubilmax Rubilmax mentioned this pull request Nov 2, 2023
Merged
@MerlinEgalite
Copy link
Contributor

@julien-devatom can you update the PR please?

@julien-devatom julien-devatom mentioned this pull request Nov 6, 2023
Closed
Rubilmax
Rubilmax suggested changes Nov 6, 2023
View reviewed changes
src/libraries/EventsLib.sol Outdated Show resolved Hide resolved
src/libraries/EventsLib.sol Outdated Show resolved Hide resolved
@QGarchery QGarchery mentioned this pull request Nov 6, 2023
Closed
@QGarchery QGarchery linked an issue Nov 6, 2023 that may be closed by this pull request
[protocol review] Make revokeRoot onlyUpdater or remove revokeRoots #68
Closed
StErMi
StErMi reviewed Nov 7, 2023
View reviewed changes
src/UniversalRewardsDistributor.sol
@@ -96,6 +97,16 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping
_setRoot(pendingRoot.root, pendingRoot.ipfsHash);
}

/// @notice Revokes the pending root.
/// @dev Can be frontrunned with `acceptRoot` in case the timelock has passed.
Copy link

Choose a reason for hiding this comment

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

I think that this natspec comment is not true anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still valid. One can trigger acceptRoot as long as block.timestamp >= pendingRoot.validAt.

@MerlinEgalite MerlinEgalite merged commit 378e73c into cantina-review Nov 7, 2023
4 checks passed
@MerlinEgalite MerlinEgalite deleted the fix/more-explicit-function-name-80 branch November 7, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StErMi StErMi StErMi left review comments

@Jean-Grimal Jean-Grimal Jean-Grimal approved these changes

@MerlinEgalite MerlinEgalite MerlinEgalite approved these changes

@QGarchery QGarchery QGarchery approved these changes

@Rubilmax Rubilmax Rubilmax approved these changes

@peyha peyha Awaiting requested review from peyha peyha was automatically assigned from morpho-org/protocol-team

@MathisGD MathisGD Awaiting requested review from MathisGD MathisGD was automatically assigned from morpho-org/protocol-team

Assignees

@julien-devatom julien-devatom

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[protocol review] Make revokeRoot onlyUpdater or remove revokeRoots
6 participants
@julien-devatom @Rubilmax @MerlinEgalite @StErMi @QGarchery @Jean-Grimal