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

Introduce min ballot duration in VotingToManageEmissionFunds contract #207

Conversation

maxaleks
Copy link
Contributor

@varasev varasev self-requested a review June 17, 2019 17:37
Copy link
Contributor

@varasev varasev left a comment

Choose a reason for hiding this comment

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

Please also review require statements related to checking the _startTime and _endTime parameters in the VotingToManageEmissionFunds.createBallot function. Since now we use the onlyValidTime modifier inside the VotingTo._createBallot internal function, those require statements in VotingToManageEmissionFunds.createBallot look redundant.

contracts/VotingToManageEmissionFunds.sol Outdated Show resolved Hide resolved
contracts/abstracts/VotingTo.sol Outdated Show resolved Hide resolved
contracts/VotingToManageEmissionFunds.sol Outdated Show resolved Hide resolved
contracts/VotingToManageEmissionFunds.sol Outdated Show resolved Hide resolved
test/voting_to_manage_emission_funds_test.js Show resolved Hide resolved
@varasev
Copy link
Contributor

varasev commented Jun 19, 2019

Let's also add canBeFinalizedNow into this condition:

if (getTotalVoters(_id) >= validatorsLength && !_withinCancelingThreshold(_id)) {
and add an additional unit tests to test/voting_to_manage_emission_funds_*.js for the VotingToManageEmissionFunds.finalize, vote, and init functions to cover the added code by tests.

@varasev
Copy link
Contributor

varasev commented Jun 21, 2019

I've checked the changes with poa-test-setup and found no problems, so I'm approving this PR.

@varasev varasev self-requested a review June 21, 2019 10:17
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