Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions contracts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ All GAS holders can vote and benefit from Neo X Governance, including EOA accoun

Neo X Governance doesn't allow voting for multiple candidates and doesn't distribute rewards to new voters until a new epoch begins. So be careful to revoke or change your vote target.

If it is necessary to change the vote target (e.g. the current voted candidate exits), invoke `transferVote(address candidateTo)` of `0x1212000000000000000000000000000000000001` to revote your deposited `GAS` to another candidate, and wait for the subsequent epoch to receive reward sharing.

At the end of every election epoch, the 7 candidates with the highest amount of votes will be selected by Governance and become consensus nodes of the next epoch. However, this consensus set recalculation has two prerequisites:

1. The size of candidate list is larger than `7`;
Expand Down
1 change: 1 addition & 0 deletions contracts/solidity/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ library Errors {
error OnlyEOA();
error InsufficientValue();
error InvalidShareRate();
error SameCandidate();
error CandidateExists();
error CandidateNotExists();
error LeftNotClaimed();
Expand Down
25 changes: 25 additions & 0 deletions contracts/solidity/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ interface IGovernance {
// revoke votes and claim rewards
function revokeVote() external;

// revoke votes, claim rewards and vote to another candidate
function transferVote(address candidateTo) external;

// only claim rewards
function claimReward() external;

Expand Down Expand Up @@ -258,6 +261,28 @@ contract Governance is IGovernance, ReentrancyGuard, UUPSUpgradeable {
_safeTransferETH(msg.sender, amount + unclaimedReward);
}

function transferVote(address candidateTo) external nonReentrant {
address candidateFrom = votedTo[msg.sender];
uint amount = votedAmount[msg.sender];
if (candidateFrom == address(0) || amount <= 0) revert Errors.NoVote();
if (candidateFrom == candidateTo) revert Errors.SameCandidate();
if (!candidateList.contains(candidateTo)) revert Errors.CandidateNotExists();

// settle reward here
uint unclaimedReward = _settleReward(msg.sender, candidateFrom);

// update votes
receivedVotes[candidateFrom] -= amount;
receivedVotes[candidateTo] += amount;
votedTo[msg.sender] = candidateTo;
voterGasPerVote[msg.sender] = candidateGasPerVote[candidateTo];
voteHeight[msg.sender] = block.number;
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest not to change the height so that if voter changes the vote target, then it still receives the reward for the rest of the epoch. Exactly like if candidate performs the second vote for the same address.

Copy link
Member

Choose a reason for hiding this comment

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

But it's a subject of discussion.

Copy link
Contributor Author

@txhsl txhsl Apr 24, 2024

Choose a reason for hiding this comment

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

There is a case that a voter first vote to candidate A whose shareRate is low (e.g. 1), then change to another candidate B whose shareRate is high (e.g. 1000) after epoch change. That could happen if A is not select as a CN, so the voter leaves because there is no reward. This seems reasonable.

But if the voter change back to A before the next epoch change. There will be a fact that the voter puts its effective votes to A, but shares rewards from B. Is that what the candidate B expects to see?

I suggest that we delay this change till the future, e.g. when we reduce the epoch length.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, your second example is reasonable, we probably need to think about this case a bit more.

I suggest that we delay this change till the future

Could you create an issue for that?

Copy link
Member

Choose a reason for hiding this comment

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

Ref. #222.


emit Revoke(msg.sender, candidateFrom, amount);
emit Vote(msg.sender, candidateTo, amount);
if (unclaimedReward > 0) _safeTransferETH(msg.sender, unclaimedReward);
}

function claimReward() external nonReentrant {
address votedCandidate = votedTo[msg.sender];
if (votedCandidate == address(0)) revert Errors.NoVote();
Expand Down
60 changes: 60 additions & 0 deletions contracts/test/Governance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,66 @@ describe("Governance", function () {
});
});

describe("transferVote", function () {
it("Should revert if the sender has not voted", async function () {
await expect(
Governance.connect(candidate1).transferVote(candidate1)
).to.be.revertedWithCustomError(Governance, ERRORS.NO_VOTE);
});

it("Should revert if the candidate from and to is the same", async function () {
await Governance.connect(candidate1).registerCandidate(500, { value: REGISTER_FEE });
await expect(
Governance.connect(candidate1).vote(candidate1, { value: MIN_VOTE_AMOUNT })
).not.to.be.reverted;

await expect(
Governance.connect(candidate1).transferVote(candidate1)
).to.be.revertedWithCustomError(Governance, ERRORS.SAME_CANDIDATE);
});

it("Should revert if the candidate to is not in the candidate list", async function () {
await Governance.connect(candidate1).registerCandidate(500, { value: REGISTER_FEE });
await expect(
Governance.connect(candidate1).vote(candidate1, { value: MIN_VOTE_AMOUNT })
).not.to.be.reverted;

await expect(
Governance.connect(candidate1).transferVote(candidate2)
).to.be.revertedWithCustomError(Governance, ERRORS.CANDIDATE_NOT_EXISTS);
});

it("Should update votes if all conditions are met", async function () {
await Governance.connect(candidate1).registerCandidate(500, { value: REGISTER_FEE });
await Governance.connect(candidate2).registerCandidate(500, { value: REGISTER_FEE });
await expect(
Governance.connect(candidate1).vote(candidate1, { value: MIN_VOTE_AMOUNT })
).not.to.be.reverted;

await expect(
Governance.connect(candidate1).transferVote(candidate2)
).not.to.be.reverted;

expect(await Governance.receivedVotes(candidate1.address)).to.eq(0);
expect(await Governance.receivedVotes(candidate2.address)).to.eq(MIN_VOTE_AMOUNT);
expect(await Governance.votedTo(candidate1.address)).to.eq(candidate2.address);
expect(await Governance.votedAmount(candidate1.address)).to.eq(MIN_VOTE_AMOUNT);
expect(await Governance.voteHeight(candidate1.address)).to.eq(await ethers.provider.getBlockNumber());
});

it("Should emit two events when a voter transfer votes", async function () {
await Governance.connect(candidate1).registerCandidate(500, { value: REGISTER_FEE });
await Governance.connect(candidate2).registerCandidate(500, { value: REGISTER_FEE });
await expect(
Governance.connect(candidate1).vote(candidate1, { value: MIN_VOTE_AMOUNT })
).not.to.be.reverted;

await expect(
Governance.connect(candidate1).transferVote(candidate2)
).emit(Governance, "Revoke").emit(Governance, "Vote");
});
});

describe("claimReward", function () {
let MockSysCall: any;
let GovReward: any;
Expand Down
1 change: 1 addition & 0 deletions contracts/test/helpers/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const ERRORS = {
ONLY_EOA: 'OnlyEOA()',
INSUFFICIENT_VALUE: 'InsufficientValue()',
INVALID_SHARE_RATE: 'InvalidShareRate()',
SAME_CANDIDATE: 'SameCandidate()',
CANDIDATE_EXISTS: 'CandidateExists()',
CANDIDATE_NOT_EXISTS: 'CandidateNotExists()',
LEFT_NOT_CLAIMED: 'LeftNotClaimed()',
Expand Down
Loading