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

Vote account parsing does not consider optional value + new account data #2013

Closed
wants to merge 2 commits into from

Conversation

ochaloup
Copy link
Contributor

@ochaloup ochaloup commented Jan 8, 2024

I troubled with an issue with the fromAccountData method


I want to deserialize data from AccountInfo.

as the deserialization was failing I needed to start checking the Solana code and I found a new version of the account is different from an "old" one.
Difference at Solana code at

Adding new account buffer was not enough as the current processing has an issue that does not consider the rootSlot as being optional (https://github.com/solana-labs/solana/blob/v1.17.15/sdk/program/src/vote/state/vote_state_1_14_11.rs#L22) and it requires it being defined all the time (https://github.com/solana-labs/solana-web3.js/blob/v1.88.0/packages/library-legacy/src/vote-account.ts#L90-L91). That's a wrong wrong deserialization processing.

I don't know what's the exactly ways around this code (it's under legacy) and I haven't tried to put into any tests.
First I would like to know if it's desirable to have the code here. Then I would like to get some feedback on the code and (if it's considered fine) then if you can share what's needed to finish the PR to be merge-able. Thanks.

@mergify mergify bot added the community label Jan 8, 2024
@mergify mergify bot requested a review from a team January 8, 2024 14:45
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Thanks for pushing, here. This situation is not great and I appreciate you taking a stab!

packages/library-legacy/src/vote-account.ts Show resolved Hide resolved
packages/library-legacy/src/vote-account.ts Outdated Show resolved Hide resolved
return new OptionLayout<T>(layout, property)
}

class OptionLayout<T> extends BufferLayout.Layout<T | null> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should also be possible to decode options with tagged unions, right? The discriminator in this case is the 0 or the 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I haven't got this. Can you please point me to some example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ochaloup
Copy link
Contributor Author

ochaloup commented Jan 19, 2024

@steveluscher awesome, thanks for your feedback and points 👍 I haven't got to this earlier but finally I have an update.

I changed to use the version parsing as you suggested. While I fight with the buffer layout a bit so not sure if it's how you like this to be. Let me know in comment if/what should be changed. Thanks again.

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants