Skip to content

Conversation

@cpubot
Copy link

@cpubot cpubot commented Nov 26, 2025

agave's Entry is comprised of a few core types exposed by the solana-sdk crate. Agave currently has its own wincode-derive implementations for those types. This PR migrates those implementations here to make this crate the source of truth for those implementations, as well as making them available to users of solana-sdk more broadly.

This isn't meant to provide comprehensive support for wincode to solana-sdk, but rather to begin introducing wincode as a feature to a few subcrates, laying the groundwork for future implementations.

Implemented types:

  • MessageHeader
  • CompiledInstruction
  • Address
  • Hash
  • Signature
  • legacy::Message
  • MessageAddressTableLookup
  • v0::Message
  • VersionedTransaction

I can also break each into a separate PR if that's preferable, though there will be some PR stacking as many of these types have dependencies in this list.

@cpubot cpubot requested a review from a team as a code owner November 26, 2025 21:55
jstarry
jstarry previously approved these changes Nov 28, 2025
buffalojoec
buffalojoec previously approved these changes Dec 1, 2025
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Super cool! The library looks awesome.

Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Sorry for the fly-by, but the code looks great to me! Just a comment on enabling deps for tests

solana-pubkey = { workspace = true, features = ["rand"] }
solana-sha256-hasher = { workspace = true }
solana-system-interface = { workspace = true, features = ["bincode"] }
solana-transaction = { path = ".", features = ["dev-context-only-utils"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might have missed something, but since the wincode feature isn't always enabled for test builds, we won't actually run the test that's gated on the wincode feature -- can you add the wincode feature here to make sure we do run the test?

Alternatively, we can add wincode to dev-context-only-utils, but I'd prefer adding the feature here since there's nothing in dev-context-only-utils that requires wincode at the moment.

Copy link
Author

Choose a reason for hiding this comment

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

Done! 07c53ae

@cpubot cpubot dismissed stale reviews from buffalojoec and jstarry via 07c53ae December 2, 2025 16:44
joncinque
joncinque previously approved these changes Dec 2, 2025
Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@cpubot cpubot requested a review from joncinque December 4, 2025 19:41
@cpubot
Copy link
Author

cpubot commented Dec 4, 2025

@joncinque -- updated to wincode v0.2.2 to leverage the safer uninit struct extensions builder (see anza-xyz/wincode#42 and anza-xyz/agave#9366) and resolved merge conflicts with master

Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

Comment on lines +3918 to +3925
[[package]]
name = "solana-short-vec"
version = "3.1.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "79fb1809a32cfcf7d9c47b7070a92fa17cdb620ab5829e9a8a9ff9d138a7a175"
dependencies = [
"serde_core",
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm only noticing this now, but we might have some issues with two versions of the same crate in our build at some point. At worst, we'll need to add a patch in Cargo.toml to force using the local solana-short-vec. We could potentially bring solana-short-vec down with the wincode crate if needed too.

Either way, it looks like we're ok for now, so nothing to do yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants