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

Arbitrum Governor V2 upgrade: contracts, scripts, tests #310

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Arbitrum Governor V2 upgrade: contracts, scripts, tests #310

wants to merge 14 commits into from

Conversation

wildmolasses
Copy link

@wildmolasses wildmolasses commented Aug 28, 2024 •
edited
Loading

To achieve the Arbitrum Governor V2 upgrade, we at ScopeLift developed contracts, scripts, and tests in a separate repo. This PR brings those contracts into the main Arbitrum governance repo. Care was taken to fit these contracts into this repo's paradigm and maintain dependencies; that said, we're super open to feedback, so if you see something in the wrong place or have another suggestion, let us know!

@wildmolasses wildmolasses marked this pull request as ready for review August 28, 2024 20:20
@wildmolasses wildmolasses changed the title Add core and treasury governor upgrade Arbitrum Governor V2 contracts, scripts, tests Aug 28, 2024
@wildmolasses wildmolasses changed the title Arbitrum Governor V2 contracts, scripts, tests Arbitrum Governor V2 upgrade: contracts, scripts, tests Aug 28, 2024
@wildmolasses wildmolasses force-pushed the 08-28-add_core_and_treasury_governor_upgrade branch from d3750a5 to f692379 Compare August 28, 2024 20:34
@gzeoneth
Copy link
Collaborator

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).

I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

@garyghayrat garyghayrat force-pushed the 08-28-add_core_and_treasury_governor_upgrade branch from 6816822 to 1067f2d Compare September 6, 2024 21:26
@garyghayrat
Copy link

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).

I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

@gzeoneth
Copy link
Collaborator

gzeoneth commented Sep 9, 2024

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it?
also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

@garyghayrat
Copy link

garyghayrat commented Sep 9, 2024 •
edited
Loading

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

@gzeoneth
Copy link
Collaborator

gzeoneth commented Sep 9, 2024

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

please look at the CI

@garyghayrat
Copy link

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

please look at the CI

Thanks! Could you check again now. FYI We need a ARBITRUM_ONE_RPC_URL set in the repo secrets because we're running fork tests.

@gzeoneth
Copy link
Collaborator

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

please look at the CI

Thanks! Could you check again now. FYI We need a ARBITRUM_ONE_RPC_URL set in the repo secrets because we're running fork tests.

We don't have access to repo settings, cc @stonecoldpat from the Arbitrum Foundation to help

@gzeoneth
Copy link
Collaborator

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

please look at the CI

Thanks! Could you check again now. FYI We need a ARBITRUM_ONE_RPC_URL set in the repo secrets because we're running fork tests.

We don't have access to repo settings, cc @stonecoldpat from the Arbitrum Foundation to help

It seems that external PR cannot read secrets, can you use envOr to default to the public Arb1 rpc?

@garyghayrat
Copy link

I noticed the Test Deployment CI is not working in this PR with some hardhat error. The hardhat build is required due to the deployment (e.g. security council governor require a solc override) and integration test dependencies. You may see the CI definition on how to replicate locally (might need to merge latest main for a recent ci fix).
I have tried to bump the hardhat version so it uses remapping.txt, but due to this open issue in hardhat NomicFoundation/hardhat#4812 context remapping which is used in the PR is not yet supported.

Hi there, just picking this up. Could you approve the workflow to run?

just did a quick check and yarn build isn't working, can you fix it? also a lot of non-contract files are included in the lib/openzeppelin-contracts-* folders, it seems better to have them removed and only keep the necessary files; a README file indicating the commit these files are copied from would also make reviewing this pr much easier

Hi, yarn build is compiling for me and there are no non-contract files in src/lib/ besides the README file I've just added. Could you try to pull the latest commit and try again. Please also share any errors you see.

please look at the CI

Thanks! Could you check again now. FYI We need a ARBITRUM_ONE_RPC_URL set in the repo secrets because we're running fork tests.

We don't have access to repo settings, cc @stonecoldpat from the Arbitrum Foundation to help

It seems that external PR cannot read secrets, can you use envOr to default to the public Arb1 rpc?

It's tough to find a rpc that can do forked fuzz tests. A few public rpcs from chainlist.org I've tried are all rate limited…

Also, any clue why Yarn Audit CI might be failing?

@gzeoneth
Copy link
Collaborator

It's tough to find a rpc that can do forked fuzz tests. A few public rpcs from chainlist.org I've tried are all rate limited…

Also, any clue why Yarn Audit CI might be failing?

Maybe the AF can create a PR on-behalf of your team so it can read secrets for CI. But I have ran the test locally and it is passing. cc @stonecoldpat

Regarding yarn audit, there is a new issue GHSA-wf5p-g6vw-rhxx that I think can be safely allowlisted.

@garyghayrat
Copy link

It's tough to find a rpc that can do forked fuzz tests. A few public rpcs from chainlist.org I've tried are all rate limited…
Also, any clue why Yarn Audit CI might be failing?

Maybe the AF can create a PR on-behalf of your team so it can read secrets for CI. But I have ran the test locally and it is passing. cc @stonecoldpat

Regarding yarn audit, there is a new issue GHSA-wf5p-g6vw-rhxx that I think can be safely allowlisted.

Hi, even though the upgrade is on hold, it would be great if we could move this PR forward. Are we now just waiting for @stonecoldpat to open a PR based on this branch?

@gzeoneth
Copy link
Collaborator

gzeoneth commented Oct 7, 2024

It's tough to find a rpc that can do forked fuzz tests. A few public rpcs from chainlist.org I've tried are all rate limited…
Also, any clue why Yarn Audit CI might be failing?

Maybe the AF can create a PR on-behalf of your team so it can read secrets for CI. But I have ran the test locally and it is passing. cc @stonecoldpat
Regarding yarn audit, there is a new issue GHSA-wf5p-g6vw-rhxx that I think can be safely allowlisted.

Hi, even though the upgrade is on hold, it would be great if we could move this PR forward. Are we now just waiting for @stonecoldpat to open a PR based on this branch?

I think so, have you verified it works locally?

wildmolasses and others added 13 commits October 7, 2024 23:36
@wildmolasses @garyghayrat
add contract
f19d639
@wildmolasses @garyghayrat
forge install: openzeppelin-contracts-v5
9cce6ef 
dbb6104
@wildmolasses @garyghayrat
forge install: openzeppelin-contracts-upgradeable-v5
300a64e 
f960f47
@wildmolasses @garyghayrat
L2ArbitrumGovernorV2 builds
ea46281 
Co-authored-by: Gary Ghayrat <[email protected]>
@wildmolasses @garyghayrat
Add supporting files
c6fbda4 
Co-authored-by: Gary Ghayrat <[email protected]>
@garyghayrat
Add onchain governor addresses to timelockRolesUpgrader test (#1)
586a732 
* Add onchain governor addrs to timelockRolesUpgrader tests

* Remove addr arguments
@garyghayrat
Add proposer &minDelay as arguments to SubmitUpgradeProposalScript (
ed01ea3 
#2)

* Add `proposer` and `minDelay` as arguments to SubmitUpgradeProposalScript

* Use proposer addresse defined in .env
@wildmolasses @garyghayrat
additional cleanup
a9d34c3
@wildmolasses @garyghayrat
move oz v5 to src/lib (#3)
475c697 
* move oz v5 to src/lib

* Build with select OZ files

* Require user to set ARBITRUM_ONE_RPC_URL

* Remove unused remapping

---------

Co-authored-by: garyghayrat <[email protected]>
@garyghayrat
Add new solidity compiler version to hardhat config
99cdd7e
@garyghayrat
Add README for src/lib/* files
940de58
@garyghayrat
Commit types folder that was initially ignore by .gitignore
d4bfedf
@garyghayrat
Update gas-snapshot
477a831
@garyghayrat garyghayrat force-pushed the 08-28-add_core_and_treasury_governor_upgrade branch from 87292fe to 477a831 Compare October 8, 2024 03:39
@garyghayrat
Copy link

Hi, even though the upgrade is on hold, it would be great if we could move this PR forward. Are we now just waiting for @stonecoldpat to open a PR based on this branch?

I think so, have you verified it works locally?

Yes, the tests are passing locally.

@garyghayrat
Copy link

Hi @stonecoldpat, would you be able to open a PR based on this branch, on our behalf, so the tests can run?

@fredlacs
Copy link
Contributor

Triggered for tests to run here, seems like some failed

@garyghayrat
Copy link

It seems that external PR cannot read secrets,

Hi, the fork tests are failing because they need an ARBITRUM_ONE_RPC_URL env. But external PRs cannot read secrets of this repo, hence we would need someone from the Arb team to open a PR on our behalf for the tests to pass.

@stonecoldpat
Copy link
Contributor

Hey, I put together a PR here:
#323

I just copied and pasted the changes into a new PR. I assume that is all that is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wildmolasses @gzeoneth @garyghayrat @fredlacs @stonecoldpat