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

Provide Governance Token Bases to TokenVotingSetup Constructor as Arguments to Avoid initCode size limit on Goerli #425

Merged
merged 4 commits into from
Jun 23, 2023
Merged

Provide Governance Token Bases to TokenVotingSetup Constructor as Arguments to Avoid initCode size limit on Goerli #425

merged 4 commits into from
Jun 23, 2023

Conversation

heueristik
Copy link
Contributor

@heueristik heueristik commented Jun 22, 2023 •
edited
Loading

Description

This PR provides the GovernanceERC20 and GovernanceWrappedERC20 token bases to the TokenVotingSetup constructor as arguments to avoid the initCode size limit on the Goerli testnet.

Note that the TokenVoting implementation remains intentionally in the constructor because the setup and implementation should never be out of sync.

Task: ID

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran all tests with success and extended them if necessary.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have updated the DEPLOYMENT_CHECKLIST file in the root folder.
  • I have updated the UPDATE_CHECKLIST file in the root folder.

@heueristik heueristik changed the base branch from develop to r/1.3.0-rc0 June 22, 2023 17:05
heueristik
heueristik commented Jun 22, 2023
View reviewed changes
packages/contracts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol
constructor(
GovernanceERC20 _governanceERC20Base,
GovernanceWrappedERC20 _governanceWrappedERC20Base
) {
tokenVotingBase = new TokenVoting();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I intentionally left new TokenVoting() here.
This is to prevent that plugins setup and implementation accidentally get out of sync.
It makes also sense because we are more likely to re-use an already deployed governance token base.

heueristik
heueristik commented Jun 22, 2023
View reviewed changes
packages/contracts/deploy/update/to_v1.3.0/41_TokenVoting_Plugin.ts
Comment on lines +32 to +47
const governanceERC20DeployResult = await deploy('GovernanceERC20', {
contract: governanceERC20Artifact,
from: deployer.address,
args: [zeroDaoAddress, emptyName, emptySymbol, emptyMintSettings],
log: true,
});

const governanceWrappedERC20DeployResult = await deploy(
'GovernanceWrappedERC20',
{
contract: governanceWrappedERC20Artifact,
from: deployer.address,
args: [zeroTokenAddress, emptyName, emptySymbol],
log: true,
}
);
Copy link
Contributor Author

@heueristik heueristik Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could re-use previously deployed GovernanceERC20 or GovernanceWrappedERC20 bases, as they didn't change (except the OZ version bump).

Should we do it? I would say it is better to pay a bit more for a fresh deployment (with the latest OZ updates) instead of having a potential address mix-up just to save a few bucks.

Other question: Will the new active_contract.json after the protocol upgrade contain the two governance token contract addresses automatically, or do we have to remember to add them there?
It could generally make sense to add all implementation/base addresses there.

@mathewmeconry @Rekard0

Copy link
Contributor

Choose a reason for hiding this comment

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

I would update them because they have changed with the OZ update.
Adding stuff to the active_contracts.json is a manual task and we can do that for sure

@heueristik heueristik marked this pull request as ready for review June 22, 2023 17:21
@github-actions github-actions bot removed the subgraph label Jun 23, 2023
Rekard0
Rekard0 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@Rekard0 Rekard0 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mathewmeconry mathewmeconry merged commit 91dc585 into r/1.3.0-rc0 Jun 23, 2023
@mathewmeconry mathewmeconry deleted the fix/init-code-reduction branch June 23, 2023 07:45
mathewmeconry added a commit that referenced this pull request Jul 11, 2023
@mathewmeconry @heueristik
R/1.3.0 rc0 (#429)
cd7c286 
* chore: updates contracts changelog with all the changes

* chore: fixes typos in UPDATE_CHECKLIST.md

* feat(rc0): managingDAO deploy proposal is now json

* fix(rc0): error in update checklist copying wrong env variables

* Provide Governance Token Bases to `TokenVotingSetup` Constructor as Arguments to Avoid `initCode` size limit on Goerli (#425)

* fix: provide governance token bases as constructor arguments

* feat: verify governance token bases

* fix: adapt script for fresh deployment

* fix: typo

* fix: includes plugin bases in contract verification

* chore(1.3.0): update active_contract.json with new addresses of testnets
chore(1.3.0): update Releases.md with new releases in testnets

* chore: update npm package versions to 1.3.0-rc0

---------

Co-authored-by: Michael Heuer <[email protected]>
mathewmeconry added a commit that referenced this pull request Jul 11, 2023
@mathewmeconry @Rekard0
@heueristik @brickpop @chilcano @josemarinas @juliettech13
merge main back (#430)
bd9ff72 
* dev2main (#397)

* OS-459 : Subgraph update to v1.3.0 (#393)

* rename 120 to 130

* fix dates

* update change log

* revert contracts test (renaming mistake)

* remove unnecessary  log

* feat: improved update checklist (#396)

* feat: improved update checklist

* feat: update PR checklist

* feat: improved the update checklist

* fix: remove checklist item

* feat: added publishing flow by Jordi

* chore: Updates update_checklist to reflect multiple version
The checkslist allows version specific tasks in a append only format

* feat: formatting

* fix: typo

* feat: restructure README section about publication

* revert: re-formatting

* fix: wrong link

---------

Co-authored-by: Mathias Scherer <[email protected]>

* Feat/update scrips (#394)

* feat: updates networks to use v1.3.0 update scripts
fix: changes mumbai rpc url to use the same infura endpoint as the others

* feat(OS-429): replaces old ipfs endpoint with new one
switches ipfs-0.aragon.network with the load balances prod.ipfs.aragon.network endpoint

* fix(OS-429): replaces old api keys with valid ones

* fix: reverts active_contracts.json to old state
the values in active_contracts.json for mumbai were already the non applied update addresses

* feat: adds node version requirement to checklists

* feat: adds new functions to deploy helpers
fix: uses correct addresses during DAOFactory v1.3.0 update

* fix: add initializeFrom call to the managing DAO proposal (#395)

* fix: add initializeFrom call to the managing DAO proposal

* fix: empty data

---------

Co-authored-by: Michael Heuer <[email protected]>

* Clarifying NatSpec comment in the `DAORegistry.register` function (#403)

* feat: added clarifying NatSpec comment

* feat: improved docs

* Tiny typo fix

---------

Co-authored-by: Jør∂¡ <[email protected]>

* feat: improved NatSpec comment (#405)

* feat: added revert case to applySingleTargetPermissions (#400)

* Dops 531 update check user perm gh actions osx (#407)

* fix(ci): new gh actions in comment-triggers to check user permission

* Added revert case to grantWithCondition (#401)

* feat: added revert case to grantWithCondition

* fix: removed empty else branch

* fix: NatSpec corrections

* fix: error usage and tests

* fix: test description

* feat: optimization

* fix: corrected build metadata (#411)

* fix: corrected build metadata

* fix: added missing item to the change list

* feat: updated deployment checklist

* feat: improved checklist item

* bump package version (#413)

* bump package version

* add satsuma ipfs gateway

* fix polygon manifest (#414)

* Added ProtocolVersion to PluginRepoFactory and PluginRepo (#412)

* feat: added ProtocolVersion to PluginRepoFactory and PluginRepo

* fix: remove wrong ERC-165 usage

* chore: maintained changelog

* add an extra check (#410)

* ERC-165 support for PermissionConditions (#402)

* feat: added revert case to grantWithCondition

* fix: removed empty else branch

* fix: NatSpec corrections

* fix: error usage and tests

* fix: test description

* feat: optimization

* feat: added PermissionConditionBase and ERC-165 support

* feat: improved error handling and additional test

* feat: improved changelog

* feat: use address instead of explicit type

* fix: tests

* feat: improved error handling

* feat: added PermissionConditionUpgradeable base contract

* fix: forgotten rename

* fix: forgotten rename

* fix: contract name and NatSpec

* fix: typo

* Update protocol upgrade deploy scripts (#409)

* feat: added deploy scripts and correction in existing scripts

* feat: maintained and improved update checklist

* fix: remove accidentally duplicated files

* revert: renaming

* feat: improved managing DAO txns descriptions

* feat: improve logs

* feat: move import

* fix: wrong checklist items

* fix: no plural

* feat: refactored tests and added contracts

* feat: improve update test

* Fix: Add decimals to ERC20WrapperContract (#416)

* add decimals to erc20wrappercontract

* update package version

* fix tests

* Documentation update: How To Guides overview (#367)

* updating documentation to be more tutorial like and use case based

* documentation update

* added fixes to docs based on code reviews

* formatting

* fixed review from michael and jordim

* Update packages/contracts/docs/osx/02-how-to-guides/index.md

* removed blank linke;

* fixing titles and revising comments on PR

* fixed michaels review

* add documentation on non-upgradeable plugin to include a get started guide

* adding the format check

* fix jordis comments on technally incorrect info and typos

* fixing plugin setup corrections

* fixed comments based on jordis input - particularly regarding the setup processing and the relationship between the plugin and the dao

* format text

* fixed jordi comments

* removing storage gap

* fixed title

* OS-516 : add missing artifacts (#408)

* add missing artifacts

* Apply suggestions from code review

Co-authored-by: Michael Heuer <[email protected]>

---------

Co-authored-by: Michael Heuer <[email protected]>

* Fix CI/CD by supporting tags (#417)

* fix: use explicit working directory paths

* Revert "fix: use explicit working directory paths"

This reverts commit 09ef365.

* feat: support tags

* fix: rename variable

---------

Co-authored-by: Rekard0 <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Jør∂¡ <[email protected]>
Co-authored-by: Roger Carhuatocto <[email protected]>
Co-authored-by: josemarinas <[email protected]>
Co-authored-by: juliettech <[email protected]>

* R/1.3.0 rc0 (#429)

* chore: updates contracts changelog with all the changes

* chore: fixes typos in UPDATE_CHECKLIST.md

* feat(rc0): managingDAO deploy proposal is now json

* fix(rc0): error in update checklist copying wrong env variables

* Provide Governance Token Bases to `TokenVotingSetup` Constructor as Arguments to Avoid `initCode` size limit on Goerli (#425)

* fix: provide governance token bases as constructor arguments

* feat: verify governance token bases

* fix: adapt script for fresh deployment

* fix: typo

* fix: includes plugin bases in contract verification

* chore(1.3.0): update active_contract.json with new addresses of testnets
chore(1.3.0): update Releases.md with new releases in testnets

* chore: update npm package versions to 1.3.0-rc0

---------

Co-authored-by: Michael Heuer <[email protected]>

* chore: remove old managingDAO address of goerli and mumbai
chore: update Changelog to reflect v1.3.0-rc0 release

---------

Co-authored-by: Rekard0 <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Jør∂¡ <[email protected]>
Co-authored-by: Roger Carhuatocto <[email protected]>
Co-authored-by: josemarinas <[email protected]>
Co-authored-by: juliettech <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathewmeconry mathewmeconry mathewmeconry approved these changes

@Rekard0 Rekard0 Rekard0 approved these changes

Assignees
No one assigned
Labels
contracts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@heueristik @mathewmeconry @Rekard0