-
Notifications
You must be signed in to change notification settings - Fork 128
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
fixed vote options serialization #560
base: main
Are you sure you want to change the base?
Conversation
@yamijuan is attempting to deploy a commit to the Solana Team on Vercel. A member of the Team first needs to authorize it. |
Deserialisation should work with this fix because of the VoteTypeKind enum. Also by setting both |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -111,8 +111,10 @@ import { deserializeBorsh } from '../tools/borsh'; | |||
writer.length += 1; | |||
|
|||
if (value.type === VoteTypeKind.MultiChoice) { | |||
writer.buf.writeUInt16LE(value.choiceCount!, writer.length); | |||
writer.length += 2; | |||
writer.buf.writeUInt8(value.choiceCount!, writer.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the struct to match the program struct https://github.com/solana-labs/solana-program-library/blob/1892778b9ddb46545f0e7982065889ee7aec1bac/governance/program/src/state/proposal.rs#L89
import { GoverningTokenConfigAccountArgs, GoverningTokenType } from '../../src'; | ||
import { BenchBuilder } from '../tools/builders'; | ||
|
||
test('setRealmConfig', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests should be in V2.V3 because they should work for spl-gov V2 and V3 and there is no V4 yet
import { GoverningTokenConfigAccountArgs, GoverningTokenType } from '../../src'; | ||
import { BenchBuilder } from '../tools/builders'; | ||
|
||
test('setRealmConfig', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this test and the one which already exists?
expect(governance.account.realm).toEqual(realm.realmPk); | ||
}); | ||
|
||
test('createProposal', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the test name more specific
Are you planning to use or already using multi choice voting? I'm asking because we are planing to make some breaking changes to it and it would be better to wait. More details here solana-labs/solana-program-library#3721 |
We aren't using multiple choice right now |
Fixed vote type serialization on Multiple choice vote type. It was using 16 bits but only sending one data, now it sends 2 option in the same 16 bits.
We also found a couple of inconsistencies!