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

Extract commitment-config crate #2136

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

kevinheavey
Copy link

@kevinheavey kevinheavey commented Jul 15, 2024

Problem

solana_sdk::commitment_config imposes a solana_sdk dependency on solana_rpc_client

Summary of Changes

  • move commitment_config.rs to its own crate
  • make serde optional in the new crate
  • re-export it in solana-sdk (with serde enabled)

@kevinheavey kevinheavey force-pushed the extract-commitment-config branch 2 times, most recently from 3f5fd99 to e1e739f Compare July 18, 2024 19:10
@kevinheavey kevinheavey force-pushed the extract-commitment-config branch 2 times, most recently from 4449f63 to 64ef4b6 Compare September 10, 2024 11:33
@kevinheavey kevinheavey requested a review from febo October 22, 2024 14:56
@kevinheavey kevinheavey force-pushed the extract-commitment-config branch 2 times, most recently from e45f3c4 to 1eb05b0 Compare October 28, 2024 09:09
@febo
Copy link

febo commented Oct 29, 2024

It seems that CommitmentConfig is always used in combination with RpcClient so I wonder whether they should be on the same crate or not. Not sure if there is a scenario where we would use CommitmentConfig and not the RpcClient.

@kevinheavey
Copy link
Author

It seems that CommitmentConfig is always used in combination with RpcClient so I wonder whether they should be on the same crate or not. Not sure if there is a scenario where we would use CommitmentConfig and not the RpcClient.

It's also used in the Client and AsyncClient traits in solana_sdk::client, and in solana-banks-client

@febo
Copy link

febo commented Oct 29, 2024

It seems that CommitmentConfig is always used in combination with RpcClient so I wonder whether they should be on the same crate or not. Not sure if there is a scenario where we would use CommitmentConfig and not the RpcClient.

It's also used in the Client and AsyncClient traits in solana_sdk::client, and in solana-banks-client

Not sure why my search did not return those uses but I can see them there. 😅

Copy link

@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.

Looks good - just waiting for the crate + CI to pass before approving.

cc: @yihau

@yihau
Copy link
Member

yihau commented Oct 30, 2024

@febo febo merged commit 96486f3 into anza-xyz:master Oct 30, 2024
52 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* extract commitment-config crate from sdk

* make serde optional in commitment-config crate

* update lock file

* remove thiserror

* remove thiserror from Cargo.toml too

* sort deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants