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

Packs certain uint256s into uint64 to save gas #473

Merged
merged 9 commits into from Oct 17, 2018

Conversation

Projects
None yet
7 participants
@IvanTheGreatDev
Copy link
Contributor

IvanTheGreatDev commented Oct 8, 2018

Fixes #461.

This reduces uint256s to uint64s where it saves gas due to the tight packing feature of Solidity.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 8, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 8, 2018

Coverage Status

Coverage decreased (-0.5%) to 96.954% when pulling 39a75c7 on IvanTheGreatDev:hotfix/reduces-uint256-to-uint64 into cdaee27 on aragon:master.

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 9, 2018

I'm not sure how coverage is decreased, can someone assist? @sohkai sorry to bother

@@ -272,7 +272,7 @@ contract Voting is IForwarder, AragonApp {
vote_.creator = msg.sender;
vote_.startDate = getTimestamp64();
vote_.metadata = _metadata;
vote_.snapshotBlock = getBlockNumber() - 1; // avoid double voting in this very block
vote_.snapshotBlock = uint40(getBlockNumber() - 1); // avoid double voting in this very block

This comment has been minimized.

@bingen

bingen Oct 9, 2018

Contributor

Why 40?

@bingen

This comment has been minimized.

Copy link
Contributor

bingen commented Oct 9, 2018

I have been looking at changes and report but I don't get it either.

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 9, 2018

Oh wait, apologies, uint40 was a change I initially made but later reverted. I'll get that fixed now.

@@ -26,11 +26,11 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
bytes32 public constant DISABLE_PAYMENTS_ROLE = keccak256("DISABLE_PAYMENTS_ROLE");

uint64 public constant MAX_PAYMENTS_PER_TX = 20;
uint64 internal constant NO_PAYMENT = 0;

This comment has been minimized.

@izqui

izqui Oct 10, 2018

Member

@IvanTheGreatDev thanks for the contribution! From my testing in the case of public constants using smaller integers makes the contract heavier, as it needs some extra code for unmasking the constants. Using the getter generated by solidity is also more expensive.

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 10, 2018

Contributor

Good point, slipped my mind! I'll get on that!

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 10, 2018

Contributor

Although, I just realized, these are internal fields, does that still apply?

I did some testing with

pragma solidity ^0.4.0; 

contract Test { 
    uint64 constant internal thing = 43; 
    uint64 constant internal thing2 = 43;
    uint64 constant internal thing3 = 43; 
    uint64 constant internal thing4 = 43; 
} 

So it turns out changing the constants uint type (with optimization enabled) has an extremely minimal effect on the gas, so because of this it's fine to make them uint256.
However, the real culprit is the public constant in the actual code, why does this need to be public? I don't see it being used anywhere. It's inclusion into the contract alone costs about 50K extra gas on deploy.

This comment has been minimized.

@izqui

izqui Oct 11, 2018

Member

With internal fields the impact is not that bad, I was actually testing the cost of using the getter generated by the public constant. I think MAX_PAYMENTS_PER_TX could be made internal as it is not used by the tests.

@sohkai what do you think? You made the majority of constants internal in #453 but not this one, was there a reason?

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

I thought it'd be useful information to be able to query for, since it's a configuration parameter that may change in the future.

I'm not opposed to making it internal though.

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 11, 2018

Contributor

If it were to be changed later we should remove constant and make a setter function for the field then no?

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

Ah, I meant across different versions of the app. Maybe later on we decide in a new version of the contract to use 50 or something. But realistically there's very little a contract or client could react to knowing the value of this parameter, and it's something really easy to expose later.

Show resolved Hide resolved apps/voting/contracts/Voting.sol

@bingen bingen dismissed their stale review Oct 10, 2018

I only had a quick glance and I guess I hit approve by mistake

Extend tests and optimize further
Also removes `artifact.json`
@sohkai
Copy link
Member

sohkai left a comment

We should double check with the gas reporter, but 👀this is looking good!

The coverage is probably just related to how coveralls calculates the coverage, as it looks like it thinks we've removed a line, so don't worry about it :).

image

@@ -85,7 +85,7 @@ contract Survey is AragonApp {
function initialize(
MiniMeToken _token,
uint256 _minParticipationPct,
uint64 _surveyTime
uint256 _surveyTime

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

I think this should still be a uint64. We should make the same changes as the Voting app here.

uint64 public voteTime;
MiniMeToken public token;

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

Let's move the token back to the top, to mirror the argument list of initialize() as well as the order it sets state. It should be the same (2 SSTOREs) either way.

@@ -272,7 +272,7 @@ contract Voting is IForwarder, AragonApp {
vote_.creator = msg.sender;
vote_.startDate = getTimestamp64();
vote_.metadata = _metadata;
vote_.snapshotBlock = getBlockNumber() - 1; // avoid double voting in this very block
vote_.snapshotBlock = uint64(getBlockNumber()) - 1; // avoid double voting in this very block

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

Let's use getBlockNumber64() for this :).

@@ -199,7 +199,7 @@ contract Voting is IForwarder, AragonApp {
}

// Voting is already decided
if (_isValuePct(vote_.yea, vote_.totalVoters, vote_.supportRequiredPct)) {
if (_isValuePct(vote_.yea, vote_.totalVoters, uint256(vote_.supportRequiredPct))) {

This comment has been minimized.

@sohkai

sohkai Oct 11, 2018

Member

I know @izqui commented on this in https://github.com/aragon/aragon-apps/pull/473/files#r224013414, but across function calls the cast is redundant as the solidity compiler automatically casts it for us.

We never actually do any math involving minAcceptQuorumPct and supportRequiredPct outside of a comparison (which would also automatically be casted for us), and it should be a red flag if we ever did.

I wouldn't mind removing the explicit upwards casts @izqui.

IvanTheGreatDev added some commits Oct 12, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 12, 2018

Just to resurface #473 (comment) (in case we need to discuss):

Across function calls the upwards casts are redundant as the solidity compiler automatically adds it for us.

We never actually do any math involving minAcceptQuorumPct and supportRequiredPct outside of a comparison (which would also automatically be casted for us), and it should be a red flag if we ever did.

I wouldn't mind removing the explicit upwards casts @izqui.

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 14, 2018

Let me know if there are any other changes needed! 😄

@chris-remus chris-remus added this to the Sprint: 2.2 milestone Oct 15, 2018

@bingen
Copy link
Contributor

bingen left a comment

Good job! And thanks for your contribution! I have some doubts that you can see inline but overall looks good!

Show resolved Hide resolved apps/finance/contracts/Finance.sol
uint256 public constant PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18
uint256 public constant ABSTAIN_VOTE = 0;
uint64 public constant PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18
uint64 public constant ABSTAIN_VOTE = 0;

This comment has been minimized.

@bingen

bingen Oct 15, 2018

Contributor

On the contrary these ones are being converted, in spite of remaining public. Doesn't that comment apply here?

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 16, 2018

Contributor

Certainly does but these are being checked inside of the test file I'm pretty sure, hence why I kept them public.

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 16, 2018

Contributor

Although it seems it's cheaper to keep them as uint256 so I'll revert that.

const holder50 = accounts[2];
const nonHolder = accounts[4];
const NULL_ADDRESS = "0x00";
const minimumAcceptanceParticipationPct = pct16(20);

This comment has been minimized.

@bingen

bingen Oct 15, 2018

Contributor

Why are those all these changes? I don't have a strong opinion about format issues, but we don't usually use trailing semicolon for JS.

This comment has been minimized.

@IvanTheGreatDev

IvanTheGreatDev Oct 16, 2018

Contributor

Ah crap that must be my linter, I'll revert them.

@IvanTheGreatDev

This comment has been minimized.

Copy link
Contributor

IvanTheGreatDev commented Oct 16, 2018

Alright, issues resolved 👍

@chris-remus

This comment has been minimized.

Copy link

chris-remus commented Oct 17, 2018

@izqui to review once more, expecting to merge after that.

@@ -127,7 +127,7 @@ contract Survey is AragonApp {
* @notice Create a new non-binding survey about "`_metadata`"
* @param _metadata Survey metadata
* @param _options Number of options voters can decide between
* @return surveyId id for newly created survey
* x@return surveyId id for newly created survey

This comment has been minimized.

@sohkai

sohkai Oct 17, 2018

Member

Small typo here, x

@@ -127,7 +127,7 @@ contract Survey is AragonApp {
* @notice Create a new non-binding survey about "`_metadata`"
* @param _metadata Survey metadata
* @param _options Number of options voters can decide between
* @return surveyId id for newly created survey
* x@return surveyId id for newly created survey

This comment has been minimized.

@izqui

izqui Oct 17, 2018

Member
Suggested change Beta
* x@return surveyId id for newly created survey
* @return surveyId id for newly created survey
@izqui

izqui approved these changes Oct 17, 2018

@bingen bingen merged commit c12c59f into aragon:master Oct 17, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details
@izqui

This comment has been minimized.

Copy link
Member

izqui commented Oct 17, 2018

Thanks for your patience, @IvanTheGreatDev! Finally merged 🎉 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment