ArbitrumFoundation / governance

Apache License 2.0
73 stars 27 forks source link

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

Open wildmolasses opened 2 months ago

wildmolasses commented 2 months ago

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!

gzeoneth commented 1 month ago

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 https://github.com/NomicFoundation/hardhat/issues/4812 context remapping which is used in the PR is not yet supported.

garyghayrat commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 2 weeks ago

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?

garyghayrat commented 2 weeks ago

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.