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

Allow Governor + CompoundTimelock to manage native tokens (eth) in and out of the timelock contract. #2849

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Sep 6, 2021

In GovernorBravo (and Alpha), the execute function must send the value attached to the proposal to the timelock

This means two things:

  • The executor must give away all the Eth needed to run the proposal
  • Any Eth present on the timelock cannot be moved away from it (such proposal would require the executor to provide an equal amount, resulting in a null result from the timelock point of view).

This means that governor + compound timelock cannot be used to manage big funds in Eth.

This PR moves away from compound behavior to enable such usecases.

Note: this issue does NOT affect OZ Governor + TimelockController combo

@Amxx Amxx changed the title Fix stupid behavior that we copied for GovernorBravo Allow Governor + CompoundTimelock to manage native tokens (eth) in and out of the timelock contract. Sep 6, 2021
@frangio frangio force-pushed the fix/compound-send-eth branch from 2ef4b15 to 41ffeb0 Compare September 6, 2021 22:59
@tarrencev
Copy link

Thanks for helping look into this. I've confirmed that our integration test passes with this change.

@frangio
Copy link
Contributor

frangio commented Sep 10, 2021

I don't think this change is ideal because it is different from the behavior of GovernorBravo in an important way: ETH sent to the Governor will be locked in it. I think this is a more general problem we should tackle for ERC20 sent to the Governor as well, although that is not a new problem with respect to GovernorBravo. I would not merge this change as is without a way to retrieve those funds.

An alternative change is to send Math.min(this.balance, values[i]). I think this results in a way to execute proposals that can use both the funds in the Governor and in the Timelock.

@Amxx
Copy link
Collaborator Author

Amxx commented Sep 10, 2021

What about the difference of behavior between our governors depending on the timelock module used? To me they are more problematic.

Also, the governor should not have a receive function if there is a timelock module (or if there is one it should revert)

@Amxx Amxx force-pushed the fix/compound-send-eth branch from b7ef479 to ae3311f Compare September 13, 2021 19:43
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Amxx Amxx merged commit 01f2ff1 into OpenZeppelin:master Sep 17, 2021
@Amxx Amxx deleted the fix/compound-send-eth branch September 17, 2021 14:57
frangio added a commit that referenced this pull request Oct 19, 2021
frangio added a commit that referenced this pull request Nov 9, 2021
frangio added a commit that referenced this pull request Nov 9, 2021
(cherry picked from commit 57630d2)
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.

3 participants