Merged
Conversation
Removed unused dependencies: - bytes: not imported anywhere (serde_bytes is used, not the bytes crate) - graphql_client: indexer uses raw GraphQL queries via reqwest, never imported - rand_core: not directly used; ed25519-dalek gets it transitively via rand - tracing-test: dev-dependency never used in any test - proptest/proptest-derive: removed from dev-deps (kept as optional for fuzzing feature) Removed unnecessary feature flags: - reqwest 'cookies': no cookie handling in the codebase - serde 'rc': no Arc/Rc serialization/deserialization - hex 'serde': hex::serialize/deserialize helpers not used - url 'serde': Url type is never serialized/deserialized via serde - syn 'extra-traits': Debug/PartialEq/Eq on syn types not used in macros crate Fixed feature definitions: - indexer feature: removed dep:graphql_client (was never used) - ed25519 feature: removed dep:rand_core (not directly needed) Performance optimizations for release profile: - Added lto = "thin" for link-time optimization - Added codegen-units = 1 for better optimization Updated dependency versions: - anyhow: 1.0.97 -> 1.0.101 - thiserror: 2.0.12 -> 2.0.17 - Ran cargo update for all compatible semver bumps Co-authored-by: Greg Nazario <greg@gnazar.io>
|
Cursor Agent can help with this pull request. Just |
- async-trait: Replaced with native async traits (Rust edition 2024). Trait methods now use 'fn -> impl Future + Send' for dyn-compatibility, and callers use 'impl Trait' generics instead of 'dyn Trait'. - futures: Replaced futures::future::join_all with tokio::task::JoinSet for concurrent task execution. tokio is already a dependency, so this eliminates the entire futures crate tree. Co-authored-by: Greg Nazario <greg@gnazar.io>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request performs a comprehensive dependency health check for the Aptos Rust SDK, focusing on removing unused dependencies, optimizing build configurations, and updating to newer dependency versions. The changes modernize the codebase by adopting native Rust async trait features and consolidating on tokio for async task management.
Changes:
- Removed unused dependencies and features (async-trait, futures, graphql_client, bytes, rand_core, proptest, tracing-test)
- Migrated from
async_traitmacro to native async fn in traits (RPITIT) for Rust 1.90+ compatibility - Replaced
futures::future::join_allwithtokio::task::JoinSetfor better async task management - Applied release profile optimizations (thin LTO, single codegen unit)
- Updated dependency versions (anyhow, thiserror, various transitive dependencies)
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/aptos-sdk/src/transaction/batch.rs | Replaced futures::join_all with tokio::JoinSet for parallel transaction submission, removed futures dependency import |
| crates/aptos-sdk/src/aptos.rs | Migrated fund_account to use tokio::JoinSet instead of futures::join_all for waiting on faucet transactions |
| crates/aptos-sdk/src/account/keyless.rs | Removed async_trait dependency, converted PepperService and ProverService traits to use native async fn in traits (RPITIT) |
| crates/aptos-sdk/Cargo.toml | Removed unused dependencies (bytes, async-trait, futures, graphql_client, rand_core) and dev-dependencies (proptest, proptest-derive, tracing-test); removed unused features from ed25519 and indexer |
| crates/aptos-sdk-macros/Cargo.toml | Removed unused "extra-traits" feature from syn dependency |
| Cargo.toml | Updated dependency versions, removed unused features (cookies, serde rc), added release profile optimizations (lto = "thin", codegen-units = 1) |
| Cargo.lock | Lockfile updates reflecting all dependency changes and version updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Update thiserror minimum version from 2.0.17 to 2.0.18 to match the version resolved in Cargo.lock. - Remove codegen-units = 1 from release profile. This is a poor default for a library crate as it significantly increases compile times (2-4x) for modest perf gains. Downstream users can set this in their own profiles. The performance/cli profiles already set it for when it's explicitly desired. Co-authored-by: Greg Nazario <greg@gnazar.io>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+213
to
217
| fn get_pepper( | ||
| &self, | ||
| jwt: &str, | ||
| ) -> impl std::future::Future<Output = AptosResult<Pepper>> + Send; | ||
| } |
There was a problem hiding this comment.
Returning impl Future from a trait method makes the trait non-object-safe, so downstream users can no longer use dyn PepperService (breaking public API). If the goal is just to remove async-trait, consider returning Pin<Box<dyn Future<Output = AptosResult<Pepper>> + Send + '_>> to keep object safety without that dependency.
Revert futures removal (keep futures::future::join_all): The futures subcrates (futures-core, futures-util, futures-channel, etc.) are already in the dependency tree transitively via reqwest -> hyper-util -> tower. Removing the direct futures dep eliminated essentially nothing from the build graph while introducing: - Task spawning overhead (JoinSet) vs zero-cost polling (join_all) - Lost insertion order requiring O(n log n) sort - Changed error semantics (dropping JoinSet aborts remaining tasks) - More complex code The tradeoff does not make sense, so join_all is restored. Fix keyless trait object safety (PepperService, ProverService): The previous impl Future return types made the traits non-object-safe, breaking downstream code using dyn PepperService / dyn ProverService. Now uses Pin<Box<dyn Future + Send + 'a>> which is exactly what async_trait desugared to, but without the proc macro dependency. This preserves the public API while still removing async-trait. Co-authored-by: Greg Nazario <greg@gnazar.io>
Co-authored-by: Greg Nazario <greg@gnazar.io>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Conduct a dependency audit to remove unused crates and features, apply release profile optimizations, and update dependency versions.