-
Notifications
You must be signed in to change notification settings - Fork 157
Deprecate re-exports more loudly #151
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
Conversation
#### Problem There are many different crates and types that we would like to remove from the sdk, but they are still re-exported from solana-sdk and solana-program. The re-exports have a `#[deprecated(...)]` attribute, but they aren't flagged to downstream users. #### Summary of changes Make the re-export deprecations louder by creating a deprecated module and re-exporting from in there. These are the types affected * `DecodeError`: this trait isn't all that useful, and is set to be removed entirely with anza-xyz#104, so deprecate it here These are the crates to no longer re-export from solana-program because they will be moved to program-specific repos: * `address_lookup_table` -> `solana_address_lookup_table_interface` * `bpf_loader_upgradeable` -> `solana_loader_v3_interface` * `loader_upgradeable_instruction` -> `solana_loader_v3_interface::instruction` * `loader_v4` -> `solana_loader_v4_interface` * `loader_v4_instruction` -> `solana_loader_v4_interface::instruction` * `nonce` -> `solana_nonce` * `feature` -> `solana_feature_gate_interface` * `loader_instruction` -> `solana_loader_v2_interface` * `vote` -> `solana_vote_interface` * `stake` -> `solana_stake_interface` * `stake_history` -> `solana_stake_interface::stake_history` * `system_instruction` -> `solana_system_interface` * `system_program` -> `solana_sdk_ids::system_program` (not sure about this one) Separately, `solana_message` and `solana_sanitize` are no longer re-exported. They never quite fit in `solana_program`, and caused more annoyance than anything else. The re-exports remain in `solana_sdk`. `solana-decode-error` will be removed entirely, so its deprecation is louder too. For `solana-sdk`, there are many types that just don't belong, so stop re-exporting these: * `commitment_config` -> `solana_commitment_config` * `genesis_config` -> `solana_genesis_config` * `hard_forks` -> `solana_hard_forks` * `rent_collector` -> `solana_rent_collector` * `alt_bn128` -> `solana_bn254` * `client` -> `solana_client_traits` * `compute_budget` -> `solana_compute_budget_interface` * `derivation_path` -> `solana_derivation_path` * `ed25519_instruction` -> `solana_ed25519_program` * `nonce_account` -> `solana_nonce_account` * `packet` -> `solana_packet` * `poh_config` -> `solana_poh_config` * `quic` -> `solana_quic_definitions` * `rent_debits` -> `solana_rent_debits` * `secp256k1_instruction` -> `solana_secp256k1_program` * `secp256k1_recover` -> `solana_secp256k1_recover` * `system_transaction` -> `solana_system_transaction` * `exit` -> `solana_validator_exit`
5972367 to
289b585
Compare
| } | ||
| #[deprecated(since = "2.2.0", note = "Use `solana-message` crate instead")] | ||
| pub use solana_message as message; | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-nonce` 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.
| #[deprecated(since = "2.3.0", note = "Use `solana-nonce` crate instead")] | |
| #[deprecated(since = "2.3.0", note = "Use `solana-system-interface::nonce` crate instead")] |
half of me wants this. other half wants to leave it separate to ease removal
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 already in a separate crate for now, but we could certainly merge them later.
| #[allow(deprecated)] | ||
| pub use solana_program::nonce::*; | ||
| } | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-sdk-ids` 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.
this crate feels like a mistake. I'd rather have individual solana_x_interface::program than a junk drawer crate
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.
I completely agree that theoretically we shouldn't need this, but you'd be surprised how many times someone just needs a pubkey and nothing else. We can certainly work towards deprecating it either way.
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.
could we not get there with features?
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.
Maybe, but it does get tricky because features should be additive. If I just want a pubkey from a crate then, all of the other functionality would be enabled on a feature, and that can get gross
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.
i guess i was thinking something like...
[features]
full = [] # everything but the declare_id!() behind this
default = [ "full" ]then if i only want pubkey...
[dependencies]
solana_crate = { version = "x.y.z", default-features = false }| #[deprecated(since = "2.2.0", note = "Use `solana-nonce-account` crate instead")] | ||
| pub use solana_nonce_account as nonce_account; | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-nonce-account` crate instead")] | ||
| pub mod nonce_account { |
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.
wouldn't solana_nonce::state be more consistent?
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.
This one's also a bit annoying, it's mostly helpers for working with solana_account types, and not actually used by the system program implementation, except in tests.
We could potentially add it to solana_nonce in the future though
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.
ah right. these are actually my fault iirc. could deprecate them entirely in theory. maybe to {solana_system_interface::,solana_}nonce::account perhaps featurized to discourage future usage
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.
Since it's already in a separate crate, we could just stop using it downstream, then we can remove the crate entirely
| #[deprecated(since = "2.2.0", note = "Use `solana-secp256k1-program` crate instead")] | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-secp256k1-program` crate instead")] | ||
| #[cfg(feature = "full")] | ||
| pub use solana_secp256k1_program as secp256k1_instruction; | ||
| #[deprecated(since = "2.1.0", note = "Use `solana-secp256k1-recover` crate instead")] | ||
| pub use solana_secp256k1_recover as secp256k1_recover; | ||
| pub mod secp256k1_instruction { | ||
| pub use solana_secp256k1_program::*; | ||
| } | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-secp256k1-recover` crate instead")] | ||
| pub mod secp256k1_recover { | ||
| pub use solana_secp256k1_recover::*; | ||
| } |
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 would also be more consistent as solana_*_interface?
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.
solana-secp256k1-program can do the same as solana-ed25519-program.
The recover crate is a bit strange, since it's just one syscall and a fallback implementation for non-solana targets. I'd prefer to keep it as is since it has some crypto deps.
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.
imo we need to work toward a consistent interface here. absurd that there's a Precompile trait and that there's like zero consistency between the implementations
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.
Yep, agreed big-time
| note = "Use `solana-system-transaction` crate instead" | ||
| )] | ||
| pub use solana_system_transaction as system_transaction; | ||
| pub mod system_transaction { |
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.
worthless helper is worthless 🪓
|
there are also a few other things we should kill
anything that lives exclusively in |
apfitzge
left a comment
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.
tx-metadata changes look good to me.
joncinque
left a comment
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 the review! To address your points:
solana_sdk::saturating_add_assign!() can be replaced by std::num::Saturating
Done, but it's gonna be loud in agave (145 uses)
solana_sdk::program_utils::limited_deserialize needs to go, but i haven't investigated why we changed the interface. i vaguely remember questioning why it exists at all
I'll make the deprecation louder, it's totally unused in agave right now
solana_sdk::rpc_port should never have existed
It's still used unfortunately, but I can add it to solana-rpc-client-api or just locally define them wherever they're used in agave.
| } | ||
| #[deprecated(since = "2.2.0", note = "Use `solana-message` crate instead")] | ||
| pub use solana_message as message; | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-nonce` 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.
It's already in a separate crate for now, but we could certainly merge them later.
| #[allow(deprecated)] | ||
| pub use solana_program::nonce::*; | ||
| } | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-sdk-ids` 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.
I completely agree that theoretically we shouldn't need this, but you'd be surprised how many times someone just needs a pubkey and nothing else. We can certainly work towards deprecating it either way.
| #[cfg(feature = "full")] | ||
| #[deprecated(since = "2.2.0", note = "Use `solana-ed25519-program` crate instead")] | ||
| pub use solana_ed25519_program as ed25519_instruction; | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-ed25519-program` 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.
This one's a bit annoying. The current crate is solana-ed25519-program, and it contains the instruction and the verification code.
However, the verification code has been deprecated in favor of agave_precompiles. Once we remove the verification code from solana-ed25519-program, we can just rename the crate to solana-ed25519-interface. What do you think?
| #[deprecated(since = "2.2.0", note = "Use `solana-nonce-account` crate instead")] | ||
| pub use solana_nonce_account as nonce_account; | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-nonce-account` crate instead")] | ||
| pub mod nonce_account { |
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.
This one's also a bit annoying, it's mostly helpers for working with solana_account types, and not actually used by the system program implementation, except in tests.
We could potentially add it to solana_nonce in the future though
| #[deprecated(since = "2.2.0", note = "Use `solana-secp256k1-program` crate instead")] | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-secp256k1-program` crate instead")] | ||
| #[cfg(feature = "full")] | ||
| pub use solana_secp256k1_program as secp256k1_instruction; | ||
| #[deprecated(since = "2.1.0", note = "Use `solana-secp256k1-recover` crate instead")] | ||
| pub use solana_secp256k1_recover as secp256k1_recover; | ||
| pub mod secp256k1_instruction { | ||
| pub use solana_secp256k1_program::*; | ||
| } | ||
| #[deprecated(since = "2.3.0", note = "Use `solana-secp256k1-recover` crate instead")] | ||
| pub mod secp256k1_recover { | ||
| pub use solana_secp256k1_recover::*; | ||
| } |
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.
solana-secp256k1-program can do the same as solana-ed25519-program.
The recover crate is a bit strange, since it's just one syscall and a fallback implementation for non-solana targets. I'd prefer to keep it as is since it has some crypto deps.
luckily i believe a majority of usage is for metrics accumulations
i have diffs that would suggest otherwise
is it used externally to agave? i'd move the whole crate back to monorepo if so. there we can eliminate it with configs and cli arg defaults as god intended |
Nice
Ah never mind, there's one usage at https://github.com/anza-xyz/agave/blob/7b368f27f69174eba154655bebd7d1775dabd3d3/core/src/banking_stage/latest_unprocessed_votes.rs#L12, all of the others use the version in
Yikes, it's very used: https://github.com/search?q=solana_sdk%3A%3Arpc_port&type=code we haven't factored it into a separate crate, but I could do that in a follow-up |
tbf, anyone who thought using this was a good idea deserves to be broken |
| pub use solana_program::loader_v4_instruction::*; | ||
| } | ||
| #[deprecated(since = "2.2.0", note = "Use `solana-message` crate instead")] | ||
| pub use solana_message as message; |
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.
We probably need to tweak this one – something like:
#[deprecated(since = "2.2.0", note = "Use `solana-message` crate instead")]
pub mod message {
pub use solana_message::*;
}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.
This one is actually on purpose -- my thinking is that Transaction / Message / Keypair / Signer are the most used types in solana-sdk, and will be the most annoying to remove, and aren't a problem to keep, so we keep it for now.
febo
left a comment
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 good to me.
apfitzge
left a comment
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.
lgtm from tx-metadata side
t-nelson
left a comment
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.
good start!
* Deprecate re-exports more loudly There are many different crates and types that we would like to remove from the sdk, but they are still re-exported from solana-sdk and solana-program. The re-exports have a `#[deprecated(...)]` attribute, but they aren't flagged to downstream users. Make the re-export deprecations louder by creating a deprecated module and re-exporting from in there. These are the types affected * `DecodeError`: this trait isn't all that useful, and is set to be removed entirely with anza-xyz#104, so deprecate it here These are the crates to no longer re-export from solana-program because they will be moved to program-specific repos: * `address_lookup_table` -> `solana_address_lookup_table_interface` * `bpf_loader_upgradeable` -> `solana_loader_v3_interface` * `loader_upgradeable_instruction` -> `solana_loader_v3_interface::instruction` * `loader_v4` -> `solana_loader_v4_interface` * `loader_v4_instruction` -> `solana_loader_v4_interface::instruction` * `nonce` -> `solana_nonce` * `feature` -> `solana_feature_gate_interface` * `loader_instruction` -> `solana_loader_v2_interface` * `vote` -> `solana_vote_interface` * `stake` -> `solana_stake_interface` * `stake_history` -> `solana_stake_interface::stake_history` * `system_instruction` -> `solana_system_interface` * `system_program` -> `solana_sdk_ids::system_program` (not sure about this one) Separately, `solana_message` and `solana_sanitize` are no longer re-exported. They never quite fit in `solana_program`, and caused more annoyance than anything else. The re-exports remain in `solana_sdk`. `solana-decode-error` will be removed entirely, so its deprecation is louder too. For `solana-sdk`, there are many types that just don't belong, so stop re-exporting these: * `commitment_config` -> `solana_commitment_config` * `genesis_config` -> `solana_genesis_config` * `hard_forks` -> `solana_hard_forks` * `rent_collector` -> `solana_rent_collector` * `alt_bn128` -> `solana_bn254` * `client` -> `solana_client_traits` * `compute_budget` -> `solana_compute_budget_interface` * `derivation_path` -> `solana_derivation_path` * `ed25519_instruction` -> `solana_ed25519_program` * `nonce_account` -> `solana_nonce_account` * `packet` -> `solana_packet` * `poh_config` -> `solana_poh_config` * `quic` -> `solana_quic_definitions` * `rent_debits` -> `solana_rent_debits` * `secp256k1_instruction` -> `solana_secp256k1_program` * `secp256k1_recover` -> `solana_secp256k1_recover` * `system_transaction` -> `solana_system_transaction` * `exit` -> `solana_validator_exit` * Address review feedback
Problem
There are many different crates and types that we would like to remove from the sdk, but they are still re-exported from solana-sdk and solana-program. The re-exports have a
#[deprecated(...)]attribute, but they aren't flagged to downstream users.Summary of changes
Make the re-export deprecations louder by creating a deprecated module and re-exporting from in there.
These are the types affected
DecodeError: this trait isn't all that useful, and is set to be removed entirely with remove solana-decode-error #104, so deprecate it hereThese are the crates to no longer re-export from solana-program because they will be moved to program-specific repos:
address_lookup_table->solana_address_lookup_table_interfacebpf_loader_upgradeable->solana_loader_v3_interfaceloader_upgradeable_instruction->solana_loader_v3_interface::instructionloader_v4->solana_loader_v4_interfaceloader_v4_instruction->solana_loader_v4_interface::instructionnonce->solana_noncefeature->solana_feature_gate_interfaceloader_instruction->solana_loader_v2_interfacevote->solana_vote_interfacestake->solana_stake_interfacestake_history->solana_stake_interface::stake_historysystem_instruction->solana_system_interfacesystem_program->solana_sdk_ids::system_program(not sure about this one)Separately,
solana_messageandsolana_sanitizeare no longer re-exported. They never quite fit insolana_program, and caused more annoyance than anything else. The re-exports remain insolana_sdk.solana-decode-errorwill be removed entirely, so its deprecation is louder too.For
solana-sdk, there are many types that just don't belong, so stop re-exporting these:commitment_config->solana_commitment_configgenesis_config->solana_genesis_confighard_forks->solana_hard_forksrent_collector->solana_rent_collectoralt_bn128->solana_bn254client->solana_client_traitscompute_budget->solana_compute_budget_interfacederivation_path->solana_derivation_pathed25519_instruction->solana_ed25519_programnonce_account->solana_nonce_accountpacket->solana_packetpoh_config->solana_poh_configquic->solana_quic_definitionsrent_debits->solana_rent_debitssecp256k1_instruction->solana_secp256k1_programsecp256k1_recover->solana_secp256k1_recoversystem_transaction->solana_system_transactionexit->solana_validator_exitI ran this branch against agave, and there won't be too many changes required. Let me know if you want to add or remove any other re-exports, these seemed like the most sensible.