getAccountInfo #3
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update dependencies and error handling across the core RPC modules while introducing a new RPC method and state management enhancements. A new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RpcHandler as Minimal RPC
participant Verifier as PubKey Verifier
participant State as State Provider
participant Compressor as Compression (zstd)
Client->>RpcHandler: get_account_info(pubkey, config)
RpcHandler->>Verifier: verify_pubkey(pubkey)
Verifier-->>RpcHandler: verification result
RpcHandler->>State: meta.get_state()
State-->>RpcHandler: current state
alt Compression requested
RpcHandler->>Compressor: compress account data
Compressor-->>RpcHandler: compressed data
end
RpcHandler-->>Client: return account info or error
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/core/src/rpc/full.rs (1)
236-269: Return transaction signature instead of "ok" string.The current implementation sends the transaction to the mempool but returns a placeholder "ok" string. As noted in the TODO comment, we should return the transaction signature.
Apply this diff to fix the return value:
- // Todo I believe we're supposed to send back a signature - Ok("ok".to_string()) + Ok(unsanitized_tx.signatures[0].to_string())
🧹 Nitpick comments (7)
crates/core/src/rpc/minimal.rs (6)
30-36: Add doc comments for clarity
Consider adding Rust doc comments (///) to explain howgetAccountInfois intended to be used, especially what kinds of accounts it supports and how edge cases are handled.
109-119: Use structured logging instead of print
Printing to stdout might clutter logs in production. Consider using a structured logging approach (e.g.,log::info!,tracing::debug!) to maintain consistency in logging levels and formats.
153-171: Log compression failures
Falling back to base64 on compression failure is good, but it may be helpful to log or differentiate compression errors for troubleshooting.- Err(_) => UiAccountData::Binary( + Err(e) => { + eprintln!("Zstd compression failed: {e}"); + UiAccountData::Binary( BASE64_STANDARD.encode(&account_data), UiAccountEncoding::Base64, ) + }
172-177: Handle unsupported encodings gracefully
Relying onunimplemented!()for all other encoding variants can cause unexpected panics. A more user-friendly error message might be appropriate.- _ => unimplemented!(), + encoding => Err(jsonrpc_core::Error::invalid_params( + format!("Encoding {encoding:?} is not supported yet.") + ))?
217-218: Optional usage ofstate_reader
let _state_reader = meta.get_state()?;is fine if you only need to confirm system health. If any additional checks are desired, referencing thestate_readercould be useful.
232-232: Consider concurrency overhead
Repeatedly callingmeta.get_state()in multiple RPC endpoints may lead to many simultaneous read locks. Typically this is fine, but keep an eye on lock contention if high concurrency is expected.Would you like support analyzing concurrency in more detail?
crates/core/src/rpc/mod.rs (1)
32-56: Trait-based state access
Introducing theStatetrait clarifies how the context retrieves shared state. This design is neat and decouples the state logic from RPC method implementations. Consider making the traitpubif external crates or modules might implement or call it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/core/Cargo.toml(1 hunks)crates/core/src/rpc/full.rs(2 hunks)crates/core/src/rpc/minimal.rs(6 hunks)crates/core/src/rpc/mod.rs(2 hunks)crates/core/src/rpc/utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
crates/core/src/rpc/minimal.rs (3)
1-1: Use of new imports
These imports (e.g.,std::io::Write,verify_pubkey,RunloopContext,base64::prelude::*,UiAccount*types) look necessary for the newly introduced account info functionality, data encoding, and verification operations. No major concerns here.Also applies to: 3-3, 5-5, 6-6, 9-9
120-126: Good fallback config handling
Defaulting toRpcAccountInfoConfig::default()when no configuration is given is a clean approach. Optionally, logging the effective config could help with troubleshooting, but this is overall fine as-is.
205-205: Consistent error handling
Callingmeta.get_state()?here nicely centralizes error handling. Good job using the new trait approach.crates/core/src/rpc/mod.rs (2)
2-2: Imports for concurrency
AddingRwLockandRwLockReadGuardis necessary for shared state access. No issues here.
9-9: RpcCustomError import
This is essential for more granular error handling. Looks good.crates/core/src/rpc/utils.rs (2)
1-1: Arc import
The addition ofArcaligns with concurrency usage in other RPC modules. No concerns.
14-14: Consolidated imports
Bringing these core type definitions (Hash, PACKET_DATA_SIZE, Pubkey, Signature, SanitizedTransaction) under one line is logically coherent. No issues.crates/core/src/rpc/full.rs (2)
5-5: LGTM!The import changes look good.
RpcContextConfigis properly used in method signatures, andsuper::*helps reduce import verbosity.Also applies to: 28-28
343-376: LGTM!The
get_latest_blockhashimplementation looks good:
- Proper error handling for meta and state read access
- Correctly constructs RpcResponse with context
- Returns latest blockhash and block height
Let's verify the block height comment. Run this script to check if we're using the correct field:
✅ Verification successful
Block Height Usage Confirmed
The block height field is used consistently across the codebase. The implementation in
crates/core/src/rpc/full.rs, which assignsstate_reader.epoch_info.block_heighttolast_valid_block_height, aligns with how the field is used in other modules.
- Confirmed usage in
crates/core/src/rpc/full.rsand similar references incrates/cli/src/tui/simnet.rsandcrates/core/src/rpc/minimal.rs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if block_height is consistently used across the codebase # Expected: Find other usages of block_height to confirm our implementation aligns with them rg -A 3 'block_height' --type rustLength of output: 1573
crates/core/Cargo.toml (1)
72-72: LGTM!The addition of
zstddependency looks good. It's pinned to a stable version and will be used for compression in theget_account_infomethod.Let's verify if this is the latest compatible version:
✅ Verification successful
Confirmed zstd Version Dependency
The dependency is correctly pinned to version 0.13.2, which is the latest stable version available according to crates.io. No further changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest zstd version and its Rust compatibility # Expected: Confirm version 0.13.2 is appropriate curl -s "https://crates.io/api/v1/crates/zstd" | jq -r '.crate.max_stable_version, .crate.documentation'Length of output: 132
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/core/src/rpc/minimal.rs (1)
151-170: Consider adding error logging for compression failures.While the implementation correctly falls back to base64 encoding when compression fails, it would be helpful to log the compression errors for debugging purposes.
match zstd::Encoder::new(&mut data, 0).and_then(|mut encoder| { encoder .write_all(&account_data) .and_then(|_| encoder.finish()) }) { Ok(_) => UiAccountData::Binary( BASE64_STANDARD.encode(&data), UiAccountEncoding::Base64Zstd, ), // Falling back on standard base64 encoding if compression failed - Err(_) => UiAccountData::Binary( + Err(e) => { + println!("Warning: zstd compression failed: {}", e); + UiAccountData::Binary( BASE64_STANDARD.encode(&account_data), UiAccountEncoding::Base64, - ), + ) + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/core/src/rpc/minimal.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
crates/core/src/rpc/minimal.rs (5)
1-24: LGTM! Import statements are well-organized.The new imports are correctly structured and necessary for the account info functionality.
109-126: LGTM! Input validation and config handling is robust.The implementation properly validates the public key and handles default configuration gracefully.
135-140: Previous out-of-bounds issue has been addressed.The implementation now safely handles data slicing with proper bounds checking.
199-234: LGTM! Error handling is now more consistent.The simplified error handling using
meta.get_state()?pattern improves code consistency and readability across methods.
187-322: Track unimplemented methods.Several methods are marked as
unimplemented!(). Consider creating tracking issues for implementing these methods:
- get_balance
- get_genesis_hash
- get_identity
- get_block_height
- get_highest_snapshot_slot
- get_transaction_count
- get_vote_accounts
- get_leader_schedule
Let's check which methods are still unimplemented:
✅ Verification successful
Track unimplemented methods.
The
crates/core/src/rpc/minimal.rsfile still contains several calls tounimplemented!(). In particular, the following methods are confirmed to be unimplemented:
get_balanceget_genesis_hashget_identityget_block_heightget_highest_snapshot_slotget_transaction_countget_vote_accountsget_leader_scheduleAdditionally, there is at least one extra call (in a match arm) that may require review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find unimplemented methods in the file rg "unimplemented!" crates/core/src/rpc/minimal.rsLength of output: 299
crates/core/src/rpc/minimal.rs
Outdated
| None => UiAccountData::Binary( | ||
| bs58::encode(account_data.clone()).into_string(), | ||
| UiAccountEncoding::Base64, | ||
| ), | ||
| _ => unimplemented!(), | ||
| } |
There was a problem hiding this comment.
Fix incorrect encoding in default case.
The default case specifies Base64 encoding but uses Base58 encoding function.
None => UiAccountData::Binary(
- bs58::encode(account_data.clone()).into_string(),
+ BASE64_STANDARD.encode(&account_data),
UiAccountEncoding::Base64,
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| None => UiAccountData::Binary( | |
| bs58::encode(account_data.clone()).into_string(), | |
| UiAccountEncoding::Base64, | |
| ), | |
| _ => unimplemented!(), | |
| } | |
| None => UiAccountData::Binary( | |
| BASE64_STANDARD.encode(&account_data), | |
| UiAccountEncoding::Base64, | |
| ), | |
| _ => unimplemented!(), | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/core/src/rpc/minimal.rs (1)
177-180:⚠️ Potential issueFix incorrect encoding in default case.
The default case specifies Base58 encoding but should use Base64 for consistency with other cases.
Apply this diff to fix the encoding:
-None => UiAccountData::Binary( - bs58::encode(account_data.clone()).into_string(), - UiAccountEncoding::Base58, -), +None => UiAccountData::Binary( + BASE64_STANDARD.encode(&account_data), + UiAccountEncoding::Base64, +),
🧹 Nitpick comments (1)
crates/core/src/rpc/minimal.rs (1)
195-205: Track unimplemented RPC methods.Several RPC methods are marked as
unimplemented!(). This could cause runtime panics if these methods are called.Would you like me to:
- Create an issue to track the implementation of these methods?
- Generate implementation templates for these methods?
The affected methods are:
get_balanceget_genesis_hashget_identityget_block_heightget_highest_snapshot_slotget_transaction_countget_vote_accountsget_leader_scheduleAlso applies to: 217-221, 230-236, 244-252, 254-261, 263-271, 283-293, 295-330
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/core/src/rpc/minimal.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
crates/core/src/rpc/minimal.rs (2)
1-37: LGTM! Clean import organization and trait definition.The imports are well-organized and the trait definition follows standard Rust patterns.
154-176: Verify zstd compression support.The code introduces zstd compression with a default compression level. Let's verify:
- The zstd dependency version
- The compression level choice
✅ Verification successful
Zstd Compression Integration Verified
- The Cargo.toml confirms the dependency is using zstd version 0.13.2.
- The code comment indicates that a default compression level of 0 is intentionally used.
- The fallback to standard Base64 encoding upon compression failure is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check zstd dependency version and compression level recommendations # Check Cargo.toml for zstd version rg -A 5 'zstd.*=' # Check if there are any discussions or recommendations about compression levels rg -i 'compression.*level|zstd.*level'Length of output: 187
crates/core/src/rpc/minimal.rs
Outdated
| ), | ||
| // Falling back on standard base64 encoding if compression failed | ||
| Err(_) => { | ||
| eprintln!("Zstd compression failed: {e}"); |
There was a problem hiding this comment.
Fix undefined error variable in error message.
The error message references an undefined variable e.
Apply this diff to fix the error:
-eprintln!("Zstd compression failed: {e}");
+eprintln!("Zstd compression failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eprintln!("Zstd compression failed: {e}"); | |
| eprintln!("Zstd compression failed"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/core/src/rpc/minimal.rs (2)
109-193: Implementation looks good with room for optimization.The implementation correctly handles:
- Data slicing with bounds checking
- Multiple encoding options (Base58, Base64, Base64Zstd)
- Error cases and compression fallback
Consider caching the capacity calculation for the compressed data vector:
- let mut data = Vec::with_capacity(account_data.len()); + // Zstd typically achieves ~2.5-3x compression ratio + let mut data = Vec::with_capacity(account_data.len() / 2);
83-102: Track technical debt for unimplemented methods.Several methods are marked with TODOs about refactoring
agave-validator wait-for-restart-window. Consider creating issues to track this technical debt.Would you like me to create issues for tracking the refactoring of these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/core/src/rpc/minimal.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
crates/core/src/rpc/minimal.rs (3)
1-24: LGTM! Import changes are well-organized.The new imports are necessary for the added functionality and properly scoped.
30-36: LGTM! Method declaration follows Solana RPC API spec.The method signature and return type are correctly defined for account info retrieval.
207-242: LGTM! Error handling improvements.The modifications streamline error handling across methods using a consistent pattern with
meta.get_state()?.
| @@ -23,6 +27,14 @@ use solana_sdk::{ | |||
| pub trait Minimal { | |||
| type Metadata; | |||
|
|
|||
| #[rpc(meta, name = "getAccountInfo")] | |||
There was a problem hiding this comment.
Instead of introducing this endpoint in Minimal, I'd be in favor of introducing the AccountsData trait, responsible for grouping a few other methods (that we can left unimplemented for now).
These traits are coming from the canonical implementation, sticking to these architecture could help with maintenance.
AccountsData introduced here: https://github.com/txtx/surfpool/pull/6/files
Summary by CodeRabbit
zstdversion0.13.2.