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/revert on already set values #92

Merged
merged 14 commits into from
Nov 8, 2023

Conversation

julien-devatom
Copy link
Contributor

  • Ive defined 2 errors since I want to distinguish the cases on submission of the pending root. Is it reverting due to the current pending root or the current root?

  • I'm not convinced about the error names, open to suggestion 😆

  • cantina issue

@julien-devatom julien-devatom requested review from a team November 1, 2023 18:03
@julien-devatom julien-devatom self-assigned this Nov 1, 2023
@julien-devatom julien-devatom requested review from Rubilmax, MerlinEgalite, Jean-Grimal, QGarchery, peyha and MathisGD and removed request for a team November 1, 2023 18:03
src/UniversalRewardsDistributor.sol Outdated Show resolved Hide resolved
src/libraries/ErrorsLib.sol Outdated Show resolved Hide resolved
test/UniversalRewardsDistributorTest.sol Outdated Show resolved Hide resolved
src/libraries/ErrorsLib.sol Outdated Show resolved Hide resolved
src/libraries/ErrorsLib.sol Outdated Show resolved Hide resolved
src/UniversalRewardsDistributor.sol Outdated Show resolved Hide resolved
test/UniversalRewardsDistributorTest.sol Show resolved Hide resolved
test/UniversalRewardsDistributorTest.sol Outdated Show resolved Hide resolved
test/UniversalRewardsDistributorTest.sol Show resolved Hide resolved
Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

Agree that it should revert on already set values, but I think that it would cleaner with #100

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder if we should make the revert explicit when the URD has already been deployed, by adding a check in the UrdFactory. I think no because it would be an edge case and also it would not be kind of awkward to do

Copy link
Contributor

Choose a reason for hiding this comment

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

@oumar-fall actually stumbled upon this issue today

src/UniversalRewardsDistributor.sol Outdated Show resolved Hide resolved
QGarchery and others added 2 commits November 7, 2023 13:43
Co-authored-by: Merlin Egalite <[email protected]>
Signed-off-by: Quentin Garchery <[email protected]>
…t-root

Streamline `submitRoot` and `setRoot`
@MerlinEgalite
Copy link
Contributor

@QGarchery maybe you can update this PR so I can trigger spearbit

Copy link
Contributor

Choose a reason for hiding this comment

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

@oumar-fall actually stumbled upon this issue today

MerlinEgalite
MerlinEgalite approved these changes Nov 7, 2023
@StErMi
Copy link

StErMi commented Nov 7, 2023

I'm not very sure how I feel about the submitRoot and setRoot implementation because the owner/updater can still call the submitRoot when the timelock == 0 and it would be immediately "acceptable".

The updater needs to always check which is the timelock value to know if it needs to call setRoot or submitRoot.

@Rubilmax
Copy link
Contributor

Rubilmax commented Nov 8, 2023

I'm not very sure how I feel about the submitRoot and setRoot implementation because the owner/updater can still call the submitRoot when the timelock == 0 and it would be immediately "acceptable".

The updater needs to always check which is the timelock value to know if it needs to call setRoot or submitRoot.

It doesn't look like an issue to me: the owner/updater would in this case have to also call acceptRoot right after
The benefit of this implementation is that each function has a single responsibility (submitRoot: manage pending root, setRoot: manage root)

@QGarchery
Copy link
Contributor

The updater needs to always check which is the timelock value to know if it needs to call setRoot or submitRoot.

The updater already had to check the timelock value because was used to discriminate whether it is possible to update instantly or not.
I think it's just better now because if the updater wants to update the root instantly, he can do so by calling setRoot. If the timelock is not 0 it would revert instead of failing silently and submitting a pending value

@MerlinEgalite MerlinEgalite merged commit 6d6a953 into cantina-review Nov 8, 2023
4 checks passed
@MerlinEgalite MerlinEgalite deleted the fix/revert-on-already-set-values branch November 8, 2023 11:19
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.

6 participants