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

Diffs for UpgradeableGHO-GHO #411

Closed

Conversation

miguelmtzinf
Copy link
Contributor

do not merge
this is just for transparency purposes

@miguelmtzinf miguelmtzinf requested a review from foodaka as a code owner June 6, 2024 10:50
Copy link

height bot commented Jun 6, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.


- constructor(string memory _name, string memory _symbol, uint8 _decimals) {
+ constructor(uint8 _decimals) {
+ decimals = _decimals;

Choose a reason for hiding this comment

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

imo it's a bit weird to have this immutable when it should always be 18.

Ofc it's quite unlikely to misconfigure, but why even add the option. Imo the path taken by oz making this a constant is more reasonable.
ref: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. However, preference is not change so the diff is little as possible

@miguelmtzinf miguelmtzinf changed the title Present diffs for UpgradeableGHO-GHO Diffs for UpgradeableGHO-GHO Jun 7, 2024
Base automatically changed from fix/upgradeable-gho-token-fixes to feat/upgradeable-gho-token June 11, 2024 08:14
@miguelmtzinf miguelmtzinf force-pushed the fix/upgradeable-gho-token-diff branch from 07bd593 to 6f75cf1 Compare June 11, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants