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

Fix doc MyGovernor example doesn't compile #4282

Merged
merged 6 commits into from
Jun 1, 2023

Conversation

qiweiii
Copy link
Contributor

@qiweiii qiweiii commented May 26, 2023

Fixes #4278

  • Put code snippet in contracts/mocks/docs so it can be checked for compilation errors.
  • Added cancel function that is missing
function cancel(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    bytes32 descriptionHash
) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) {
    return super.cancel(targets, values, calldatas, descriptionHash);
}

PR Checklist

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

@changeset-bot
Copy link

changeset-bot bot commented May 26, 2023

⚠️ No Changeset found

Latest commit: 6009e8a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@frangio
Copy link
Contributor

frangio commented May 29, 2023

Thank you @qiweiii! Would you mind doing the same for the other Solidity contracts in governance.adoc?

@qiweiii
Copy link
Contributor Author

qiweiii commented May 30, 2023

Yea, happy to do it

@qiweiii
Copy link
Contributor Author

qiweiii commented May 30, 2023

@frangio Just added other solidity contract example, but I have a few concerns:

  1. example contract naming could be confusing in future if move more example under contracts/mocks/docs, for example, there would be many MyToken in docs, instead of naming them MyToken1, MyToken2, do you think specific name like MyTokenGovernance could be better?
  2. example code format is changed by solidity code formatter, after putting them in .sol file
function _mint(address to, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._mint(to, amount);
    }

is changed to

function _mint(address to, uint256 amount) internal override(ERC20, ERC20Votes) {
    super._mint(to, amount);
}

making the line a bit longer, user will need to scroll in order to read, is this acceptable?

@frangio
Copy link
Contributor

frangio commented May 30, 2023

  1. Perhaps we can put them under mocks/docs/governance/ to disambiguate that they are for the governance article. MyTokenGovernance is not necessary then. But it would still make sense to have different names that refer to the specific thing the example is showcasing.
  2. Oh, this is not ideal but I don't know how to fix it so for now let's just accept what the formatter wants.

@frangio
Copy link
Contributor

frangio commented May 30, 2023

I realized mocks/docs/governance/ would not have worked with one of our scripts so I pushed a commit to fix that.

@qiweiii
Copy link
Contributor Author

qiweiii commented May 30, 2023

we can put them under mocks/docs/governance/ to disambiguate that they are for the governance article.

Good idea!
I have put them under /governance, do you think the new contract names are good?

@qiweiii
Copy link
Contributor Author

qiweiii commented May 30, 2023

test-upgradable failed:

<--- Last few GCs --->

[1930:0x4e72540]  1644838 ms: Scavenge (reduce) 4063.6 (4098.7) -> 4062.7 (4098.7) MB, 8.4 / 0.0 ms  (average mu = 0.369, current mu = 0.359) allocation failure 
[1930:0x4e72540]  1644943 ms: Scavenge (reduce) 4063.6 (4095.7) -> 4062.7 (4096.7) MB, 18.3 / 0.0 ms  (average mu = 0.369, current mu = 0.359) allocation failure 
[1930:0x4e72540]  1645059 ms: Scavenge (reduce) 4063.7 (4095.7) -> 4062.7 (4096.7) MB, 11.3 / 0.0 ms  (average mu = 0.369, current mu = 0.359) allocation failure 


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

do we need to increase the --max_old_space_size?

# .github/workflows/checks.yml
- NODE_OPTIONS: --max_old_space_size=4096
+ NODE_OPTIONS: --max_old_space_size=5120

@frangio frangio changed the base branch from master to release-v4.9 May 31, 2023 23:23
@frangio
Copy link
Contributor

frangio commented May 31, 2023

I've retargeted the branch to release-v4.9 and rebased on top of that because there are some new changes in master that affect this and we will want to update separately. I will open a separate PR to include the changes in master.

Thank you so much!

frangio
frangio previously approved these changes May 31, 2023
@Amxx
Copy link
Collaborator

Amxx commented Jun 1, 2023

This PR includes some changes to ERC721Consecutive (the offset part). I'm not sure its intentional. I also don't think it should be the case.

@frangio frangio dismissed their stale review June 1, 2023 14:30

Bad changes in PR

@frangio
Copy link
Contributor

frangio commented Jun 1, 2023

My bad, happened when I rebased... Will fix later.

@frangio frangio merged commit 8198205 into OpenZeppelin:release-v4.9 Jun 1, 2023
@gitpoap-bot
Copy link

gitpoap-bot bot commented Jun 1, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

GitPOAP: 2023 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

frangio pushed a commit to frangio/openzeppelin-contracts that referenced this pull request Jun 1, 2023
Co-authored-by: Francisco Giordano <fg@frang.io>
(cherry picked from commit 8198205)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix docs snippets that don't compile
3 participants