-
Notifications
You must be signed in to change notification settings - Fork 254
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
Extract solana-transaction from solana-sdk #3634
Conversation
3186d52
to
303be04
Compare
0dcdc95
to
936e4af
Compare
936e4af
to
b778d4e
Compare
86eeea0
to
dcd740a
Compare
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @kevinheavey. |
c750530
to
275a26f
Compare
3b6ec80
to
f347973
Compare
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.
Thanks for taking this one on too. The separation of precompiles
and verify
makes sense, we should just use solana_system_interface
and solana_message
instead of solana_program
sdk/instruction/Cargo.toml
Outdated
@@ -48,7 +48,7 @@ serde = [ | |||
std = [] | |||
|
|||
[package.metadata.docs.rs] | |||
targets = ["x86_64-unknown-linux-gnu"] | |||
targets = ["x86_64-unknown-linux-gnu", "wasm32-unknown-unknown"] |
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 this change for?
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.
Can't remember why it happened in this PR but it is a good change, otherwise you can't see the wasm-specific code on docs.rs
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.
it's possible i meant to add it for the solana-transaction crate. But it matters for solana-instruction too
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.
If it's not needed, let's make the change in a separate PR. That way we can add it to all of the affected crates at once. I put in #3901 for it
@@ -0,0 +1,21 @@ | |||
#![cfg(feature = "full")] | |||
#[deprecated(since = "2.2.0", note = "Use solana_transaction_error crate instead")] |
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.
nit: all of these re-exports except for TransactionResult
were already deprecated as of 2.1.0, but it's fine to bump this up for simplicity
a65746c
to
183dcf3
Compare
@apfitzge I see that you are the owner of the empty solana-transaction crate: https://crates.io/crates/solana-transaction/settings Could you please add |
Yeah, I'll transfer ownership when I get to my desk |
@yihau I added |
#### Problem As pointed out in [this PR comment](anza-xyz#3634 (comment)) it's good to specify that docs build for wasm32 where needed. Also, solana-sdk and solana-program don't configure docs builds with all features enabled. #### Summary of changes In most places, add "wasm32-unknown-unknown" as a build target. For sdk and program, also configure the features to be enabled.
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.
Really close, just one last little nit and the bit about solana-instruction's Cargo.toml
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.
Looks great, thanks!
#### Problem As pointed out in [this PR comment](#3634 (comment)) it's good to specify that docs build for wasm32 where needed. Also, solana-sdk and solana-program don't configure docs builds with all features enabled. #### Summary of changes In most places, add "wasm32-unknown-unknown" as a build target. For sdk and program, also configure the features to be enabled.
Problem
solana_sdk::transaction
needs to be pulled out ofsolana_sdk
Summary of Changes
solana_sdk::simple_vote_transaction_checker
to this crate (not much choice here as there'd be a circular dep otherwise)solana_sdk::transaction::Result
tosolana_transaction_error::TransactionResult
and re-export insolana_sdk::transaction
with deprecation. This should have been done before but it was quite noticeable when moving things that it clearly belongs insolana_transaction_error
verify_precompiles
methods behind a feature gate "precompiles", as these bring in extra dependenciesverify
andverify_*
methods behind a feature gate "verify", as they bring in extra dependencies (not as many as the precompiles though)The new crate still depends indirectly on serde and bincode for now because it depends on solana-programThis branches off #3622 so that needs to be merged first (update: done)