Skip to content

Conversation

@yeastplume
Copy link
Member

This PR updates the request and tokio dependecies to latest, and hyper to match Grin upstream

  • Upgraded tokio to version 1.x with full features enabled
  • Updated reqwest to version 0.12
  • Updated hyper dependency to match grin upstream
  • Modified master build to use grin master

@yeastplume yeastplume requested a review from Copilot March 30, 2025 12:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the Tokio, Reqwest, and related dependencies to their latest versions and aligns the Grin dependencies with the upstream master branch. Key changes include updating dependency versions, modifying build configurations, and refactoring runtime usage in the client module.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
util/Cargo.toml Commented out fixed version and enabled master branch dependency for grin_util
src/build/build.rs Removed unused dependency and updated argument passing syntax
libwallet/src/mwixnet/onion/crypto/dalek.rs Added dead code annotations for unused functions
libwallet/src/lib.rs Adjusted re-exports and removed duplicate crate usage
libwallet/Cargo.toml Updated Grin dependency definitions to point to master branch
integration/Cargo.toml Upgraded hyper, futures, HTTP, and tokio dependencies
impls/src/client_utils/client.rs Modified the runtime creation to align with Tokio 1.0 improvements
impls/Cargo.toml Upgraded tokio and reqwest versions and adjusted Grin dependency settings
controller/src/controller.rs Updated hyper usage to correctly call to_bytes
controller/Cargo.toml Upgraded dependency versions including tokio and grin dependencies
config/src/types.rs Simplified default implementations for TorBridgeConfig and TorProxyConfig
config/src/config.rs Replaced clone patterns with more idiomatic code and improved config path handling
config/src/comments.rs Cleaned up comment migration code by refining the iterator operations
config/Cargo.toml Updated Grin dependency definitions to point to master branch
api/Cargo.toml Updated Grin dependency definitions to point to master branch
Cargo.toml Updated the root Grin dependency definitions to point to master branch
Comments suppressed due to low confidence (1)

impls/src/client_utils/client.rs:29

  • Now that Tokio 1.0 allows Runtime::block_on to be called without mutable access, consider removing the Mutex wrapper around RUNTIME to simplify the code.
pub static ref RUNTIME: Arc<Mutex<Runtime>> = Arc::new(Mutex::new(

@Anynomouss
Copy link
Contributor

Anynomouss commented May 3, 2025

Looks all good. I guess that means I have to update my pull request #732 since it involves some changes in config.rs. Any tips on how to to integrate these changes not manually? I am not exactly a github wizard.

@Anynomouss
Copy link
Contributor

Anynomouss commented Sep 23, 2025

I used the updates from this PR in my pull request #732
Meaning that this PR should be merged first, and my PR732 should be merged on top.

Files that include the changes from this PR in PR732 are listed below:
config/src/config.rs [includes changes from this PR]
controller/src/comments.rs [includes changes from this PR]

impls/src/lifecycle/default.rs
impls/src/lifecycle/seed.rs
src/bin/grin-wallet.rs
src/cmd/wallet_args.rs
impls/src/node_clients/http.rs
tests/common/mod.rs

There should be no conflicts to my knowledge if merged in the proposed order.

Review:
Since this PR only updates existing libraries, the only review I could do is run all test and check if the code made sense. I had no issue compiling the code, all test passed as expected and changes improve the readability of the logic in the code. The only issue I can image that might occur is when using an old versions of Rust. I am happy to see that those unused function in dalek.rs are marked as dead code, it always bugged me to get those warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants