Make ERC20Wrapper.underlying variable private #4029
Conversation
Removed the public visibility from the underlying variable Refactored the underlying variable to _underlying
Implemented the underlying function to return the value of the _underlying variable
Bugfix: ERC20Wrapper underlying variable is public
🦋 Changeset detectedLatest commit: 55aa375 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Included the private visibility to the _underlying variable Pulled the latest changes
|
@Amxx Thanks, I went ahead and made the suggested changes |
Amxx
changed the title
|
@Amxx Should this function be virtual? If so, should we use the function instead of reading the variable? I honestly don't think anyone should override this, if it was an individual choice I wouldn't make it virtual. Also, this is technically breaking, but I don't think it will affect anyone, we should still add a small note in the changelog. |
Whatever is decided, I think it'll apply to 3863 as well. Is that the case? |
|
Changing the value returned by this function would break everything. That is why its currently implemented with |
frangio
changed the title
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2023 OpenZeppelin Contracts Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
|
@Amxx @frangio @ernestognw I appreciate merging the changes and responding to the questions I had. I'm looking forward to contributing more in the future, thank you |
Fixes #4026
Changes
IERC20 immutable _underlying;and refactored it to include the_due to linting errors.underlying() public viewfunction to return the value of the_underlyingvariable.underlyingfunction.Visual Preview
PR Checklist
npx changeset add)