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

Fixed Misleading Typo in CHANGELOG.md related to false solidity version #4697

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

0xprinc
Copy link
Contributor

@0xprinc 0xprinc commented Oct 22, 2023

Fixes #????

PR Checklist

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

@changeset-bot
Copy link

changeset-bot bot commented Oct 22, 2023

⚠️ No Changeset found

Latest commit: e62c667

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

@Amxx
Copy link
Collaborator

Amxx commented Oct 23, 2023

Hello @0xprinc

I don't understand this requested change. All files in the release branch use pragma solidity ^0.8.20.

The only file in our codebase that doesn't use this pragma is contracts/mocks/governance/GovernorStorageMock.sol. That is a mock file, that is not released and should have no effect on the user.

@0xprinc
Copy link
Contributor Author

0xprinc commented Oct 26, 2023

Hello sir,
Actually there is a typo that I have reported.
Its written
Bumped minimum compiler version required to 0.8.20
But instead it should be 0.8.19 as the link to the pull request this hyperlink is mentioning to is
Bump minimum pragma version to 0.8.19

Actually this typo got me to get false insights about the smart contract I was doing security analysis of.

@Amxx
Copy link
Collaborator

Amxx commented Oct 26, 2023

I think there is a missunderstanding here.

Yes, the PR #4288 did update the minimum version to 0.8.19 ... but then we did another PR that changed that to 0.8.20.
We decided to not have 2 CHANGELOG entries, to avoid confusion ... and instead we updated the entry that was already there.

Maybe we should change the PR link to the latest one (the one that targets 0.8.20).

CHANGELOG.md Outdated
@@ -48,7 +48,7 @@ These removals were implemented in the following PRs: [#3637](https://github.com
#### General

- Replaced revert strings and require statements with custom errors. ([#4261](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4261))
- Bumped minimum compiler version required to 0.8.20 ([#4288](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4288))
- Bumped minimum compiler version required to 0.8.19 ([#4288](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4288))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Bumped minimum compiler version required to 0.8.19 ([#4288](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4288))
- Bumped minimum compiler version required to 0.8.20 ([#4489](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4489))

Copy link
Member

Choose a reason for hiding this comment

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

We should reference both PRs in my opinion.

Suggested change
- Bumped minimum compiler version required to 0.8.19 ([#4288](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4288))
- Bumped minimum compiler version required to 0.8.20 ([#4489](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4489)) ([#4288](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4288))

@Amxx Amxx requested a review from ernestognw October 26, 2023 14:55
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@ernestognw
Copy link
Member

Thanks!

@ernestognw ernestognw merged commit 94697be into OpenZeppelin:master Oct 26, 2023
16 of 17 checks passed
@gitpoap-bot
Copy link

gitpoap-bot bot commented Oct 26, 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.

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.

3 participants