-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feat/get account info #10
Conversation
WalkthroughThe changes update the account retrieval and transformation logic along with the RPC configuration management. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as AccountsDataRpc
participant SR as StateReader (svm)
participant RPC as RemoteRpcClient
participant U as UI Transformation
C->>R: get_account_info(pubkey)
R->>SR: Fetch account by pubkey
alt Account exists
SR-->>R: Return account data
R->>U: Transform account data
U-->>R: UiAccount
else Account missing
R->>RPC: Request account from remote RPC
RPC-->>R: Return account data or error
R->>U: Transform fetched account data
U-->>R: UiAccount
end
R-->>C: Return UiAccount or error
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
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 (
|
@@ -122,7 +149,10 @@ impl AccountsData for SurfpoolAccountsDataRpc { | |||
.iter() | |||
.map(|s| { | |||
let pk = verify_pubkey(s)?; | |||
transform_account_to_ui_account(&state_reader.svm.get_account(&pk), &config) | |||
match state_reader.svm.get_account(&pk) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be hitting RPC here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we haven't done this method yet, this was just making the compiler happy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/core/src/rpc/mod.rs (1)
88-96
: Consider caching the RPC client.Creating a new RPC client for each request might be inefficient. Consider caching the client instance.
- fn get_remote_rpc_client(&self) -> Result<RpcClient, RpcCustomError> { - let Some(ctx) = self else { - return Err(RpcCustomError::NodeUnhealthy { - num_slots_behind: None, - } - .into()); - }; - Ok(RpcClient::new(&ctx.rpc_config.remote_rpc_url)) + fn get_remote_rpc_client(&self) -> Result<RpcClient, RpcCustomError> { + let Some(ctx) = self else { + return Err(RpcCustomError::NodeUnhealthy { + num_slots_behind: None, + } + .into()); + }; + static RPC_CLIENT: OnceCell<Mutex<Option<RpcClient>>> = OnceCell::new(); + let client_mutex = RPC_CLIENT.get_or_init(|| Mutex::new(None)); + let mut client = client_mutex.lock().unwrap(); + if client.is_none() { + *client = Some(RpcClient::new(&ctx.rpc_config.remote_rpc_url)); + } + Ok(client.as_ref().unwrap().clone()) + }crates/core/src/rpc/accounts_data.rs (2)
94-97
: Consider using structured logging.Replace
println!
with a proper logging framework for better observability in production.- println!( - "account from state: {:?}", - state_reader.svm.get_account(&pubkey) - ); + tracing::debug!( + account = ?state_reader.svm.get_account(&pubkey), + "Retrieved account from state" + );
152-155
: Consider implementing parallel account retrieval.For multiple accounts, consider implementing parallel retrieval using async/await for better performance.
- match state_reader.svm.get_account(&pk) { - Some(account) => Ok(Some(transform_account_to_ui_account(&account, &config)?)), - None => Ok(None), - } + tokio::spawn(async move { + match state_reader.svm.get_account(&pk) { + Some(account) => Ok(Some(transform_account_to_ui_account(&account, &config)?)), + None => Ok(None), + } + }).await?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/core/src/rpc/accounts_data.rs
(3 hunks)crates/core/src/rpc/mod.rs
(4 hunks)crates/core/src/rpc/utils.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_cargo_checks
- GitHub Check: build
🔇 Additional comments (3)
crates/core/src/rpc/mod.rs (1)
32-33
: LGTM! Good addition of RPC configuration.The addition of
rpc_config
toRunloopContext
enables better RPC configuration management.crates/core/src/rpc/accounts_data.rs (1)
107-117
: Uncomment and implement account caching.The commented-out caching logic should be implemented to improve performance for frequently accessed accounts.
Would you like me to help implement a proper caching mechanism with TTL and size limits?
crates/core/src/rpc/utils.rs (1)
174-234
: LGTM! Good improvement in type safety.The change to require a non-optional
Account
parameter improves type safety and simplifies the function's logic.
Closing in favor of #9 |
Summary by CodeRabbit