Skip to content

move less important methods from Sysvar to SysvarSerialize #87

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kevinheavey
Copy link
Contributor

@kevinheavey kevinheavey commented Mar 26, 2025

Problem: the Sysvar trait relies on bincode, but most of the time people just want Sysvar::get which doesn't rely on bincode

Solution: make a SysvarGet trait that has one method: get. Make the Sysvar trait use SysvarGet::get. Annoyingly this is still technically a breaking change because we are adding a constraint to Sysvar, but for the vast majority of users nothing should break

Solution: move the other methods out of Sysvar and into a new SysvarSerialize trait

Copy link
Contributor

@febo febo left a comment

Choose a reason for hiding this comment

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

This change is great! I wonder if we could take the opportunity of this breaking change to do a bigger refactoring. Ideally we should move the implementation of each sysvar to their own individual crate, instead of having all of them as a dependency on this one.

So solana-sysvar has the trait definitions and macros only. What do you think?

Another alternative is to move the trait definitions and macros to a new crate (solana-define-sysvar?), move the implementation to each individual sysvar crate and keep this one re-exporting everything to "minimize" the breaking change.

@kevinheavey
Copy link
Contributor Author

Sounds like another PR to me

@febo
Copy link
Contributor

febo commented Apr 2, 2025

Sounds like another PR to me

Yeah, definitely. I was mentioning to get an idea if this is an approach that makes sense. I am inclined to go for the solana-define-sysvar option, which in a way mimics the syscalls approach.

@kevinheavey
Copy link
Contributor Author

The solana-define-sysvar approach is a bit awkward because of the orphan rule. We'd have to move the trait impls to the existing crates like solana-clock. But maybe that's pretty cheap once we have this lightweight SysvarGet trait

@febo
Copy link
Contributor

febo commented Apr 4, 2025

The solana-define-sysvar approach is a bit awkward because of the orphan rule. We'd have to move the trait impls to the existing crates like solana-clock. But maybe that's pretty cheap once we have this lightweight SysvarGet trait

Yeah, the new trait is lightweight and the impl could be behind a feature on the individual crates.

@@ -145,11 +142,24 @@ pub fn is_sysvar_id(id: &Pubkey) -> bool {
ALL_IDS.iter().any(|key| key == id)
}

/// Interface for loading a sysvar.
pub trait SysvarGet: Default + Sized {
Copy link
Contributor

Choose a reason for hiding this comment

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

We removed the requirement for the Default trait in pinocchio, so I wonder if we should do the same here as well. Individual sysvars could still implement Default when applicable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should do this in a separate PR since we would need to modify the impl_sysvar_get macro implementation.

Copy link
Contributor

@febo febo left a comment

Choose a reason for hiding this comment

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

Overall the approach is an improvement over the current bincode requirement for Sysvar, but I wonder whether it would be better to refactor the whole Sysvar trait instead of creating a new trait. We should stop using bincode anyway, so we could just get rid of that.

@kevinheavey
Copy link
Contributor Author

I don't think the other methods on Sysvar belong in the same trait as get:

  • 99% of use cases just need get
  • even if bincode is replaced (which will take a while), there will still be a significant amount of code involved in implementing those methods

@febo
Copy link
Contributor

febo commented Jul 4, 2025

I don't think the other methods on Sysvar belong in the same trait as get:

  • 99% of use cases just need get
  • even if bincode is replaced (which will take a while), there will still be a significant amount of code involved in implementing those methods

Agree. I should have been more specific when I said "refactor" Sysvar. I was thinking extracting the bincode related methods to a new trait and leave the get on the Sysvar. This will make it easier to eventually deprecate bincode without having to deprecate the Sysvar trait, which is a good trait name.

In other words, inverting the changes of the PR: the new SysvarGet stays as Sysvar and current Sysvar is renamed to SysvarSerialize/SysvarDeserialize (maybe a better name) with the bincode functionality.

@kevinheavey
Copy link
Contributor Author

oh ok yeah makes sense

Copy link
Contributor

github-actions bot commented Jul 4, 2025

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

Copy link
Contributor

github-actions bot commented Jul 4, 2025

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

@kevinheavey kevinheavey changed the title factor out SysvarGet from Sysvar move less important methods from Sysvar to SysvarSerialize Jul 4, 2025
@joncinque joncinque added the breaking PR contains breaking changes label Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking PR contains breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants