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

replace solana_sdk::pubkey with solana_pubkey in ledger #4235

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

kevinheavey
Copy link

Problem

This crate uses solana_sdk in many places. It will take multiple PRs to replace it everywhere

Summary of Changes

Find and replace solana_sdk::pubkey with solana_pubkey

@@ -2,7 +2,7 @@ use {
itertools::Itertools,
rand::distributions::{Distribution, WeightedIndex},
rand_chacha::{rand_core::SeedableRng, ChaChaRng},
solana_sdk::pubkey::Pubkey,
solana_pubkey::Pubkey,
Copy link

@steviez steviez Jan 3, 2025

Choose a reason for hiding this comment

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

I think we can make this substitution on a bunch more files in solana_ledger. Looks like:

  • benches/blockstore.rs
  • src/blockstore.rs
    • This file looks to have a redundant import in the test module as well
  • src/blockstore_db.rs
  • src/blockstore_processor.rs
    • Redundant import in test module as well
  • src/genesis_utils.rs
  • src/leader_schedule.rs
    • Already done per the highlighted line in the diff
  • src/leader_schedule_cache.rs
  • src/leader_schedule_utils.rs
  • src/shred.rs
  • src/shredder.rs
  • src/sigverify_shreds.rs
  • src/staking_utils.rs
  • src/token_balances.rs
  • src/transaction_address_lookup_table_scanner.rs
  • src/shred/merkle.rs

Copy link
Author

Choose a reason for hiding this comment

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

let's do this in a separate PR?

Copy link

Choose a reason for hiding this comment

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

I'm somewhat inclined to include it in this same PR. The PR title states "replace solana_sdk::pubkey with solana_pubkey in ledger", and without these changes, that is not fully accomplished. The diff should still be pretty small with these changes. Finally, if we were to do those changes in a separate PR, then I'd argue we should revert this line in src/leader_schedule.rs for the sake of consistency across the crate.

That being said, I don't want to punish a good deed. So, if you're opposed to tagging the change onto this PR, I'd probably still be fine pushing the PR.

Copy link
Author

Choose a reason for hiding this comment

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

ah by that PR title I meant sed -i 's/solana_sdk::pubkey/solana_pubkey/g'

@kevinheavey kevinheavey force-pushed the ledger-replace-pubkey-import branch from 89dc08a to 94e9da1 Compare January 4, 2025 11:29
@steviez steviez merged commit f594a32 into anza-xyz:master Jan 6, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants