Use TIMELOCK_ADDRESS as proxy owner in deploy script
#34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wildmolasses
requested changes
Oct 22, 2024
| @@ -13,13 +13,15 @@ contract CompoundGovernorTest is Test, CompoundGovernorConstants { | |||
| IComp token; | |||
| ICompoundTimelock timelock; | |||
| address owner; | |||
| address proxyAdmin; | |||
There was a problem hiding this comment.
Suggested change
| address proxyAdmin; | |
| address proxyAdminOwner; |
| address whitelistGuardian; | ||
| CompoundGovernor.ProposalGuardian proposalGuardian; | ||
| uint96 constant PROPOSAL_GUARDIAN_EXPIRY = 1_739_768_400; | ||
|
|
||
| function setUp() public virtual { | ||
| // set the owner of the governor (use the anvil default account #0, if no environment variable is set) | ||
| owner = vm.envOr("DEPLOYER_ADDRESS", 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266); | ||
| proxyAdmin = makeAddr("PROXY_ADMIN_ADDRESS"); |
There was a problem hiding this comment.
Suggested change
| proxyAdmin = makeAddr("PROXY_ADMIN_ADDRESS"); | |
| proxyAdmin = COMPOUND_TIMELOCK; |
script/DeployCompoundGovernor.s.sol
Outdated
| { | ||
| function run( | ||
| address _owner, | ||
| address _proxyAdmin, |
There was a problem hiding this comment.
Suggested change
| address _proxyAdmin, | |
| address _compoundTimelock, |
script/DeployCompoundGovernor.s.sol
Outdated
| @@ -46,7 +48,7 @@ contract DeployCompoundGovernor is Script, CompoundGovernorConstants { | |||
| ) | |||
| ); | |||
| TransparentUpgradeableProxy _proxy = | |||
| new TransparentUpgradeableProxy(address(_implementation), address(this), _initData); | |||
| new TransparentUpgradeableProxy(address(_implementation), _proxyAdmin, _initData); | |||
There was a problem hiding this comment.
Suggested change
| new TransparentUpgradeableProxy(address(_implementation), _proxyAdmin, _initData); | |
| new TransparentUpgradeableProxy(address(_implementation), _compoundTimelock, _initData); |
garyghayrat
force-pushed
the
feat/accept-proxy-admin-in-script
branch
from
0491481 to
54de5ad
Compare
garyghayrat
changed the base branch from
feat/add-whitelists-and-guardians
to
main
wildmolasses
approved these changes
Oct 23, 2024
script/DeployCompoundGovernor.s.sol
Outdated
| { | ||
| function run( | ||
| address _owner, | ||
| address _compoundTimelock, |
There was a problem hiding this comment.
sorry @garyghayrat, one more nit, if we already have the CompoundGovernorConstants.TIMELOCK_ADDRESS, why not use it here instead of passing in the timelock as an arg?
There was a problem hiding this comment.
hah, great point 92f02b7
Pull Request Test Coverage Report for Build 11483322570Details
💛 - Coveralls |
wildmolasses
changed the title
proxyAdmin as constructor arg in deploy scriptTIMELOCK_ADDRESS as proxy owner in deploy script
wildmolasses
pushed a commit
that referenced
this pull request
Nov 1, 2024
* Accept `proxyAdmin` as constructor arg in deploy script * Make timelock `proxyAdminOwner` and rename vars * Remove proxyAdminOwner as an arg
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.