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

[N-09] Naming issues #433

Merged
merged 1 commit into from
Jun 23, 2022
Merged

[N-09] Naming issues #433

merged 1 commit into from
Jun 23, 2022

Conversation

scott-silver
Copy link
Contributor

Some naming suggestions from OZ, if we choose to accept them

  • replaced all UInts with Uint
  • renamed TransparentUpgradeableConfiguratorProxy to ConfiguratorProxy
  • replaced rescale and descale variables with a shared DESCALE constant

I did not take their suggestion re: CometProxyAdmin. Personally, I disagree with their reasoning. To me, the name CometProxyAdmin means that it is Comet's ProxyAdmin, not that it is solely the Admin of the CometProxy.

@scott-silver scott-silver added the openzeppelin PRs and issues related to OZ audit label Jun 23, 2022
jflatow
jflatow reviewed Jun 23, 2022
View reviewed changes
contracts/Comet.sol Outdated Show resolved Hide resolved
jflatow
jflatow reviewed Jun 23, 2022
View reviewed changes
contracts/Comet.sol Outdated Show resolved Hide resolved
jflatow
jflatow reviewed Jun 23, 2022
View reviewed changes
contracts/Comet.sol Outdated Show resolved Hide resolved
jflatow
jflatow reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@jflatow jflatow left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab 🔪

@kevincheng96
Copy link
Contributor

Agree with you Scott that CometProxyAdmin can be interpreted as the "proxy admin for all of Comet". In terms of the other changes, I'm okay with all of them but think the Uint one is probably not necessary (reasoning here).

@scott-silver
Copy link
Contributor Author

Seems like the only change with clear value is TransparentUpgradeableConfiguratorProxy -> ConfiguratorProxy.

I'll rebase and drop the UInt vs Uint and descale/rescale changes.

@scott-silver scott-silver force-pushed the silver/oz-n-09_naming-issues branch from c41216d to 4029d83 Compare June 23, 2022 20:20
@jflatow jflatow merged commit b5a64d0 into main Jun 23, 2022
MichaelMorami pushed a commit to Certora/comet that referenced this pull request Jun 29, 2022
@scott-silver @MichaelMorami
Rename TransparentUpgradeableConfiguratorProxy to ConfiguratorProxy (c…
18d0eaa 
…ompound-finance#433)
@jflatow jflatow mentioned this pull request Jul 1, 2022
Closed
@jflatow jflatow deleted the silver/oz-n-09_naming-issues branch August 4, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jflatow jflatow jflatow approved these changes

@toninorair toninorair Awaiting requested review from toninorair

@kevincheng96 kevincheng96 Awaiting requested review from kevincheng96

Assignees
No one assigned
Labels
openzeppelin PRs and issues related to OZ audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scott-silver @kevincheng96 @jflatow