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

Update MixinVoting.sol #311

Closed
wants to merge 1 commit into from
Closed

Conversation

0xScratch
Copy link

Changed uint256 i = 0; to uint256 i;

Default value of all 'uint' used to be 0. Thus, it's kind of a wastage of some gas when we initialize some 'uint' variable to 0

@gabririgo
Copy link
Contributor

please note it is not best practice to leave uninitialized variables due to possible bugs in the compiler. Also when opening PRs, please conform to Conventional Commits. Closing.

@gabririgo gabririgo closed this Sep 11, 2023
@0xScratch
Copy link
Author

Well, Kind of noticed many projects using this convention, like at least not initializing some unsigned integers with 0.
Moreover, Can you explain a little more why you think this might create some possible bugs in the compiler. Cuz it's quite clear if you don't initialize a 'uint' with anything, it got automatically the value of '0'.

Btw you can take a look at (see the overall optimized for loop that guy is using) -> ethereum/solidity#10695

@gabririgo
Copy link
Contributor

we tend to stick to what is commonly best practice, see Openzeppelin governor for reference.
I completely agree that not initializing the first unsigned integer with 0 should have no impact, there have been cases in the past where bugs in the compiler that were unknown at the time of deploying a contract have resulted in unexpected smart contract behavior. In this particular case, the decision to stick to initializing the variable was due to weighting the risk of unforeseen behavior by using a slightly different approach than the standard and the benefits in terms of gas savings, which are minimal. Thank you for checking in.

@0xScratch
Copy link
Author

Well, In that case, makes sense!
Thanks

@0xScratch 0xScratch deleted the patch-1 branch September 12, 2023 15:49
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