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

Move creator, metadata, and reference storage to event data #434

Merged
merged 12 commits into from Oct 23, 2018

Conversation

@wjmelements
Copy link
Contributor

wjmelements commented Sep 11, 2018

Changes

This moves creator and metadata to event data for both Vote and Survey because it isn't used anywhere.
It isn't especially important who created the vote or survey after initial validation.
For client interfaces, we still want to note the creator, but the appropriate place to do that is event data.
This saves a minor amount of gas, and, eventually, rent.
Reviewers @izqui

@wjmelements

This comment has been minimized.

Copy link
Contributor

wjmelements commented Sep 11, 2018

Pull wjmelements#1 to also relocate the metadata, which seems to not be used on-chain.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 11, 2018

@wjmelements Debating whether or not we might need this info on-chain, even if we don't use it nor have anyone relying on it right now.

I can't think up of any particular case where another contract might care off the top of my head, but this definitely feels like something one could come up with an example of (@izqui @bingen any ideas?).

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 12, 2018

Thanks for bringing this up @wjmelements. I actually don't think we need those on-chain so they can be safely moved into a log, specially creator which isn't being used anywhere.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 12, 2018

We could also remove this from the Survey app.

Merge pull request #1 from wjmelements/voting-metadata-event
Also move metadata to the start event
@wjmelements

This comment has been minimized.

Copy link
Contributor

wjmelements commented Sep 12, 2018

I will make similar changes in the survey contract then.

@wjmelements wjmelements changed the title Move vote creator to event data Move vote creator and metadata to event data Sep 12, 2018

@wjmelements

This comment has been minimized.

Copy link
Contributor

wjmelements commented Sep 12, 2018

Further changes are required in app/src/script.js to fetch the data from the event logs.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 17, 2018

Just realized, the Finance app also has an instance of creator and reference that we might be able to remove to save gas.

@izqui izqui requested a review from bingen Oct 15, 2018

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

@bingen

bingen approved these changes Oct 15, 2018

Copy link
Contributor

bingen left a comment

Looks good to me. Thanks for the contribution! I'll fix merge conflicts to be able to merge.

@@ -5,6 +5,7 @@ const timeTravel = require('@aragon/test-helpers/timeTravel')(web3)
const getContract = name => artifacts.require(name)
const pct16 = x => new web3.BigNumber(x).times(new web3.BigNumber(10).toPower(16))
const createdSurveyId = receipt => receipt.logs.filter(x => x.event == 'StartSurvey')[0].args.surveyId

This comment has been minimized.

@bingen

bingen Oct 15, 2018

Contributor

Maybe you could get rid of this one and just use the newly created one below

@izqui

izqui approved these changes Oct 16, 2018

Copy link
Member

izqui left a comment

LGTM, thanks for the contribution @wjmelements, this is going to save so much gas :D

@sohkai

sohkai approved these changes Oct 16, 2018

Copy link
Member

sohkai left a comment

🛢 🔥

@chris-remus

This comment has been minimized.

Copy link

chris-remus commented Oct 17, 2018

Pending decision, do this for Finance? @bingen @sohkai @izqui to talk and decide later today.

Move reference fields to event data
Address PR #434 comments.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2018

Coverage Status

Coverage decreased (-0.05%) to 96.907% when pulling 9eea949 on wjmelements:voting-creator-event into 50f6a3b on aragon:master.

@@ -674,14 +667,13 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
transaction.token = _token;
transaction.entity = _entity;
transaction.date = getTimestamp64();
transaction.reference = _reference;

This comment has been minimized.

@sohkai

sohkai Oct 18, 2018

Member

@bingen It might be nice to re-order this to reflect the order of the the Transaction struct itself. Right now the storage is being set out of order, and I wouldn't count on the compiler to optimize it correctly for us.

Same in Voting too :).

This comment has been minimized.

@sohkai

sohkai Oct 18, 2018

Member

@bingen I've done this in dcf3544.

Rudimentary gas testing on the Voting app shows 15k decreases in gas for affected tests.

@sohkai

sohkai approved these changes Oct 18, 2018

@sohkai sohkai changed the title Move vote creator and metadata to event data Move creator, metadata, and reference storage to event data Oct 19, 2018

@sohkai sohkai merged commit 50a841a into aragon:master Oct 23, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.05%) to 96.907%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 23, 2018

Finally merging this breaking change @wjmelements! Thanks for all your help!

@wjmelements

This comment has been minimized.

Copy link
Contributor

wjmelements commented Oct 23, 2018

Hurray!
I'm sorry I didn't have time to finish this.

sohkai added a commit that referenced this pull request Oct 25, 2018

Update app scripts (#520)
Updates the apps after the contract changes in #434.

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