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

Add state getter in TimelockController using OperationState enum #4358

Merged
merged 24 commits into from
Jul 4, 2023

Conversation

RenanSouza2
Copy link
Contributor

@RenanSouza2 RenanSouza2 commented Jun 16, 2023

Fixes #4316
Fixes LIB-899

TimelockController

  • remove:

  • isOperationPending

  • isOperationReady

  • isOperationDone

  • implement:

    • getOperationState

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2023

🦋 Changeset detected

Latest commit: 8d43f44

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@RenanSouza2 RenanSouza2 changed the title first proposal Add state getter in TimelockController using OperationState enum Jun 16, 2023
@frangio
Copy link
Contributor

frangio commented Jun 28, 2023

To reiterate, what is missing in this PR is to add back isOperationReady, etc., and I would add: to implement those functions in terms of getOperationState() so that getOperationState is the "source of truth".

# Conflicts:
#	contracts/governance/extensions/GovernorTimelockControl.sol
@RenanSouza2
Copy link
Contributor Author

RenanSouza2 commented Jun 28, 2023

And what is de decision for the pending name?

I suggest keep the pending meaning of 'operation scheduled but not done or canceled' because of the previous method 'isOperationPending'. And to create another name fot the state between scheduled but before the time alloed to execute like 'blocked'

@frangio
Copy link
Contributor

frangio commented Jun 29, 2023

Pending should preserve the meaning implied by isOperationPending in 4.x code.

I suggest the name pending be replaced by 'blockded' or something similar

I agree with this proposal, we can have OperationState.Blocked in the enum.

@RenanSouza2
Copy link
Contributor Author

RenanSouza2 commented Jun 29, 2023

  1. In line 341 I wasn't sure in what to include in the error because Pending is no longer in the enum

  2. I used the isOperation<...> inside the contract in case of override, let me know what you think

@frangio
Copy link
Contributor

frangio commented Jun 29, 2023

  1. In line 341 I wasn't sure in what to include in the error because Pending is no longer in the enum

Interesting. The issue is that cancel can be done in two states, and the TimelockUnexpectedOperationState error only has a single OperationState expected parameter. Should we do the same as with the GovernorUnexpectedProposalState error and use a bitmap? @Amxx @ernestognw What do you think?

@ernestognw
Copy link
Member

Interesting. The issue is that cancel can be done in two states, and the TimelockUnexpectedOperationState error only has a single OperationState expected parameter. Should we do the same as with the GovernorUnexpectedProposalState error and use a bitmap? @Amxx @ernestognw What do you think?

I think what we did in GovernorUnexpectedProposalState is the easiest way of expressing multiple enum values so I agree. Alternatively, I'd use the earliest state (eg. blocked) as the required state because it's first in the lifecycle of an operation.

More in favor if a bitmap anyway.

@Amxx
Copy link
Collaborator

Amxx commented Jun 30, 2023

ok with bitmap

@RenanSouza2
Copy link
Contributor Author

As of now I just copied the '_encodeStateBitmap' from 'Governor' to 'TimelockController' I would like to unify both implementation, maybe in a file in the utils folder in governance itself, but this woul affect too many files so I want to see yout take on that before doing it

@frangio
Copy link
Contributor

frangio commented Jul 3, 2023

I would like to unify both implementation

I don't think we can do this because it's a different enum type.

This was referenced Sep 10, 2024
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.

Add state getter in TimelockController using OperationState enum
4 participants