Anyone should be able to activate an account's vote

[deepakhb2]

Description

Currently an accounts pending votes can be activated by the same account. This change will allow to activate any other accounts pending vote. But, the account should belong to group i.e is passed. A new method is added to implement this change and non of the existing methods are modified.

Other change

Upgraded Election contract to inherit InitializableV2.

Tested

Added unit tests to cover this change.

Related issues

• Fixes Anyone should be able to activate an account's vote #6467

Backwards compatibility

As this change is made by added a new method and it doesn't affect the existing features.

[deepakhb2]

@nambrot The tests are failing due to ElectionTestInstance interface don't have the new property that was added as part of this change. Any idea where this interface is defined? It looks like this is defined in some other repo.

[nambrot]

Hmm that is a good question. It's based upon https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/contracts/governance/test/ElectionTest.sol#L9:L9 which should inherit the method and therefore should have it during type generation

[nambrot]

Is it passing locally for you?

[AlexBHarley]

Nice! I'm a big fan of this functionality, it will be a great UX improvement for staking services.

[AlexBHarley]

I'm all for more tests but I feel this logic is already tested by the #activate.when another voter activates votes test.

[deepakhb2]

This is already tested by activate for negative scenarios. In this case it positive test cases for activateByAccount method.

[deepakhb2]

Thank you @nambrot for pointing me to the file how the types are generated and I had made typo while calling the method from test.
I tried running the tests locally and I see more fails with other issues. Also, I am not sure if I used the right command to run the tests locally.

[nambrot]

What command are you using to run the test? For the initializable error, consider checking out #7254, you just have to make Election inherit from the new InitializableV2 library instead of the old one

[deepakhb2]

@nambrot I tried running tests using npm run test command.

[deepakhb2]

@m-chrzan I think ElectionTestContract needs some update. I am not sure if InitializableV2 is creating issues during build. https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/test/governance/voting/lockedgold.ts#L33 please let me know if I am missing something here. Thank you.

[m-chrzan]

@m-chrzan I think ElectionTestContract needs some update. I am not sure if InitializableV2 is creating issues during build. https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/test/governance/voting/lockedgold.ts#L33 please let me know if I am missing something here. Thank you.

Huh, so this appears to be a slight bug in the test that hadn't been caught before. To fix this particular error message, it should be enough to change that line to

const Election: ElectionContract = artifacts.require('Election')

(since we're requireing the Election contract, not the ElectionTest one) and then propagating those changes throughout the file (changing imports, changing references to ElectionTest to Election).

I imagine you might then start running into issues with the ElectionTest contract itself, since it inherits from Election (which now inherits from InitializableV2), so also needs to call its parent constructor. Since ElectionTest will always be used in testing, just doing

contract ElectionTest is Election(true) {

should be enough.

[deepakhb2]

@m-chrzan I think ElectionTestContract needs some update. I am not sure if InitializableV2 is creating issues during build. https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/test/governance/voting/lockedgold.ts#L33 please let me know if I am missing something here. Thank you.

Huh, so this appears to be a slight bug in the test that hadn't been caught before. To fix this particular error message, it should be enough to change that line to

const Election: ElectionContract = artifacts.require('Election')

(since we're requireing the Election contract, not the ElectionTest one) and then propagating those changes throughout the file (changing imports, changing references to ElectionTest to Election).

I imagine you might then start running into issues with the ElectionTest contract itself, since it inherits from Election (which now inherits from InitializableV2), so also needs to call its parent constructor. Since ElectionTest will always be used in testing, just doing

contract ElectionTest is Election(true) {

should be enough.

Thank you @m-chrzan for sharing more details around this issue with solution.

[deepakhb2]

@nambrot One required 'certora-test' is failing. Is it related to this PR?

[yorhodes]

@nambrot One required 'certora-test' is failing. Is it related to this PR?

@deepakhb2 This is unrelated and there was a fix merged to master today. I have updated your branch.

[yorhodes]

1 nit

[nambrot]

@deepakhb2 Looks like there is a linting error?

[deepakhb2]

@nambrot for election.ts file the prettier check is getting passed on my local machine. I am not sure if this is with election.ts file.

[yorhodes]

@nambrot for election.ts file the prettier check is getting passed on my local machine. I am not sure if this is with election.ts file.

Patch for the lint fix
https://gist.github.com/yorhodes/1181c8d1cd64426f2a4af529a64ef3ec

Generated from
yarn prettier -w --config .prettierrc.js packages/protocol/test/governance/voting/election.ts

[nambrot]

@yorhodes @deepakhb2 Can one of y'all apply the patch, and if it passes then I can merge

[deepakhb2]

@nambrot I have applied the patch and lint-checks is passed.