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

chore: Improve Logging, RPC Extension, and JSON-RPC Compatibility #14150

Closed
wants to merge 1 commit into from

Conversation

mdqst
Copy link
Contributor

@mdqst mdqst commented Feb 1, 2025

Fixed a few inconsistencies and improved overall clarity:

Better Logging with tracing

  • Replaced println!("txpool extension enabled"); with tracing::info! for structured logging.
  • Added tracing_subscriber::fmt::init(); to initialize proper logging.

Optimized RPC Module Extension

  • Removed unnecessary if !args.enable_ext { return Ok(()); }, making the logic cleaner.
  • The RPC module is now added only when args.enable_ext == true.

Fixed JSON-RPC API Types

  • Changed usize to u64 for better JSON-RPC client compatibility.

Improved CLI Argument Parsing

  • Cleaned up struct definitions by separating #[derive(...)] from #[arg(long)].

Enhanced Tests with a Mock Transaction Pool

  • Introduced MockTransactionPool, replacing NoopTransactionPool.
  • Updated assertions to better simulate real scenarios.

@mdqst mdqst requested a review from gakonst as a code owner February 1, 2025 12:08
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

hmm, these changes don't add additional value


ctx.modules.merge_configured(ext.into_rpc())?;
tracing::info!("txpool extension enabled"); // Improved logging
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tracing::info!("txpool extension enabled"); // Improved logging
tracing::info!("txpool extension enabled");

Comment on lines -27 to -29
if !args.enable_ext {
return Ok(())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer this because this removes one indent

Comment on lines -31 to -36
// here we get the configured pool.
let pool = ctx.pool().clone();

let ext = TxpoolExt { pool };

// now we merge our extension namespace into all configured transports
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to keep these docs because this is an example and these provide helpful context

@mattsse mattsse closed this Feb 1, 2025
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