Implement voteOnProposal command #220
Conversation
Irfan-Personal
requested review from
sergeyshemyakov,
emiliolisk and
Daniel-Lightcurve
|
|
||
| public commands = []; | ||
| public commands = [this.__createvoteOnPorposalCommand]; |
There was a problem hiding this comment.
Please fix the typos to "Proposal" and add proper caps/names where needed, for example this would be this._voteOnProposalCommand
There was a problem hiding this comment.
yes it was not aligned with command name so change it to
this.__createvoteOnPorposalCommand => __voteOnPorposalCommand at the required places in code.
I will push the code soon.
|
|
||
| const index = ctx.params.proposalIndex; | ||
|
|
||
| const stakedAmount = (await this._posEndpoint.getLockedStakedAmount(moduleEndpointContext)) |
There was a problem hiding this comment.
Seems it was already added as a method here, I think it would make more sense to use that
There was a problem hiding this comment.
@emiliolisk I think I am already using the method you mentioned here. I have an import at the top of file as well:
import { PoSEndpoint } from 'lisk-framework/dist-node/modules/pos/endpoint';
Can you please give a look again. Thanks
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| methodContext, | ||
| this.stores.get(ProposalsStore), | ||
| index, | ||
| voteInfos[itr]!.amount, |
There was a problem hiding this comment.
This should be -amount, i.e. we deduce the previously cast votes from the total result.
There was a problem hiding this comment.
@sergeyshemyakov true replaces this line with:
-voteStoreInfos[itr]!.amount,
| voteInfos[itr]!.amount, | ||
| voteInfos[itr]!.decision, | ||
| ); | ||
| votesStoreInfo.setKey(methodContext, [senderAddress], newVoteInfo); |
There was a problem hiding this comment.
If I understand the code correctly, there is an error here. I think you set the votes store entry to newVoteInfo, but voteInfos is an array that should mostly stay the same i.e. the entries for this address for other votes should not be deleted. You should reassign voteInfos[itr] = newVoteInfo.voteInfos[0].
There was a problem hiding this comment.
@sergeyshemyakov
Updated this and replaced.
votesStoreInfo.setKey(methodContext, [senderAddress], newVoteInfo);
=>
voteInfos[itr] = newVoteInfo.voteInfos[0];
|
|
||
| if ( | ||
| !previousSavedStorescheck && | ||
| (await votesStoreInfo.get(methodContext, senderAddress)).voteInfos.length < |
There was a problem hiding this comment.
Just a small suggestion: you could use voteInfos.length here.
There was a problem hiding this comment.
@sergeyshemyakov updated it to your recommendation.
| ); | ||
| } else if (!previousSavedStorescheck) { | ||
| [(await votesStoreInfo.get(methodContext, senderAddress)).voteInfos[smallestproposalIndex]] = | ||
| newVoteInfo.voteInfos; |
There was a problem hiding this comment.
I think it should be newVoteInfo.voteInfos[0].
There was a problem hiding this comment.
@sergeyshemyakov agree and that is why changed it to:
[voteStoreInfos[smallestproposalIndex]] = newVoteInfo.voteInfos;
it is doing array destructing in TS and it is equal to:
voteStoreInfos[smallestproposalIndex] = newVoteInfo.voteInfos[0];
| @@ -51,3 +51,5 @@ export const DECISION_NO = 1; // Code for the vote decision "No". | |||
| export const DECISION_PASS = 2; // Code for the vote decision "Pass". | |||
|
|
|||
| export const defaultConfig = {}; | |||
|
|
|||
| export const COMMAND_ID_VOTE_ON_PORPOSAL = Buffer.from('0002', 'hex'); | |||
There was a problem hiding this comment.
We don't have command IDs anymore, instead we have command names:
COMMAND_VOTE_ON_PROPOSAL | string | "voteOnProposal" | Command name of the vote command.
There was a problem hiding this comment.
@sergeyshemyakov chnaged it to:
export const COMMAND_VOTE_ON_PORPOSAL = 'voteOnProposal';
| @@ -38,7 +38,7 @@ export const ProposalVotedEventSchema = { | |||
| }, | |||
| voterAddress: { | |||
| dataType: 'bytes', | |||
| length: LENGTH_ADDRESS, | |||
There was a problem hiding this comment.
The address of the voter should be exactly 20 bytes (i.e. LENGTH_ADDRESS). So the previous version was correct.
There was a problem hiding this comment.
@sergeyshemyakov if change it to length it gives a schema validator error
Rejected to value: [Error: Lisk validator found 1 error[s]: strict mode: unknown keyword: "length"]
| expect(result.error?.message).toBe('Decision does not exist'); | ||
| expect(result.status).toEqual(VerifyStatus.FAIL); | ||
| }); | ||
| it('should Fail with Proposal does not exist message', async () => { |
There was a problem hiding this comment.
I would add a test case for a revote. Like this: cast a vote with x tokens and decision No, recast a vote from the same account with x tokens and decision Yes. Check that votesYes for the proposal increased by x while votesNo decreased by x.
There was a problem hiding this comment.
@sergeyshemyakov I agree and that's why I have updated the tests and included these lines:
The bases value for votesYes and votesNo is 100;
expect((await dexGovernanceStore.getKey(methodContext, [indexBuffer])).votesNo).toBe( BigInt(95), ); expect((await dexGovernanceStore.getKey(methodContext, [indexBuffer])).votesYes).toBe( BigInt(205), );
What was the problem?
This PR resolves #126 for the dexGovernace module.
How was it solved?
voteOnProposalcommand in voteOnProposal.ts file.How was it tested?