Fetch and save missing accounts #9
Conversation
|
Warning Rate limit exceeded@lgalabru has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request modifies asynchronous account fetching and RPC client usage. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountsData
participant LocalState
participant RPCClient
Client->>AccountsData: get_account_info(pubkey)
AccountsData->>LocalState: Check if account exists
alt Account Found
LocalState-->>AccountsData: Return account
AccountsData->>Client: Encode and return account data
else Account Not Found
AccountsData->>RPCClient: Fetch account asynchronously
RPCClient-->>AccountsData: Return fetched account data
AccountsData->>LocalState: Save account data
AccountsData->>Client: Encode and return account data
end
sequenceDiagram
participant Simnet
participant RPCClient
participant RemoteServer
Simnet->>RPCClient: Initialize non-blocking client with remote URL
Simnet->>RPCClient: get_epoch_info().await
RPCClient->>RemoteServer: Send async request for epoch info
RemoteServer-->>RPCClient: Return epoch info
RPCClient-->>Simnet: Deliver epoch info
Possibly related PRs
Poem
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
🧹 Nitpick comments (4)
crates/core/src/simnet/mod.rs (3)
56-57: Consider robust error handling or fallback for RPC unavailability.Right now, if
get_epoch_info().await?fails, the entire startup process may abort. Adding retries or more granular error handling ensures resilience if the remote RPC endpoint is temporarily unavailable.
65-65: Placing the non-blocking client in global state may impact future extensibility.While sharing the
Arc<RpcClient>is convenient, it tightly couples your global logic to a single client configuration. Consider whether constructing or injecting a client on demand would be cleaner for testing or future scaling.
123-123: Fetching each missing account individually may introduce performance bottlenecks.Performing
get_account(&program_id).awaitin a loop could be expensive, especially if multiple accounts are missing. You might consider batching strategies or pre-fetching. If accounts are frequently absent, a bulk request or caching layer could reduce latency.crates/core/src/rpc/accounts_data.rs (1)
148-220: Batch-fetch approach is beneficial, but partial errors are handled silently.
- Ignoring RPC fetch errors: Similar to
get_account_info, the code at line 172 uses.await.ok(). This means any error is coerced intoNonewith no user-visible explanation. Consider returning or logging errors for better observability.- Concurrency or multiple fetch attempts: If multiple threads call
get_multiple_accountsconcurrently for overlapping sets of accounts, you might fetch the same missing accounts multiple times. Caching or a lock-based approach can reduce redundant calls.- UI encoding*: The asynchronous approach is properly finalizing the transformation with
encode_ui_account. This is good, but confirm you handle large or streaming data gracefully.Proposed handling of fetch errors (near line 172):
- async move { (pk, rpc_client.get_account(&pk).await.ok()) } + async move { + match rpc_client.get_account(&pk).await { + Ok(acc) => (pk, Some(acc)), + Err(e) => { + warn!("Failed to fetch account {}: {:?}", pk, e); + (pk, None) + } + } + }
📜 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/utils.rs(1 hunks)crates/core/src/simnet/mod.rs(4 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/simnet/mod.rs (1)
6-6: Adopting an async RPC client and introducing an RwLock look good overall.This change enables non-blocking calls and controlled concurrent access to shared state. Ensure that shared data remains consistent (e.g., watch out for potential read/write conflicts) and confirm that your concurrency model won't lead to deadlocks under high load.
Also applies to: 12-12
crates/core/src/rpc/accounts_data.rs (1)
1-1: New imports align with asynchronous logic and UI account encoding needs.These additions (e.g.,
Arc,verify_pubkey,join_all,Error,encode_ui_account,UiAccountEncoding) support the new async methods and the UI account transformation approach.Also applies to: 3-3, 6-6, 8-8, 11-11, 17-17
crates/core/src/rpc/utils.rs (1)
1-1: Removal of UI account transformation references and slight import updates.These import adjustments reflect the removal of
transform_account_to_ui_accountand reliance on direct usage ofencode_ui_accountinaccounts_data.rs. Everything seems consistent with the new async logic.Also applies to: 7-7
| let config = config.unwrap_or_default(); | ||
| let pubkey = match verify_pubkey(&pubkey_str) { | ||
| Ok(res) => res, | ||
| Err(e) => return Box::pin(future::err(e)), | ||
| }; | ||
|
|
||
| let config = config.unwrap_or_default(); | ||
| let state_reader = match meta.get_state() { | ||
| Ok(res) => res, | ||
| Err(e) => return Box::pin(future::err(e.into())), | ||
| }; | ||
|
|
||
| let res = match transform_account_to_ui_account( | ||
| &state_reader.svm.get_account(&pubkey), | ||
| &config, | ||
| ) { | ||
| Ok(res) => Ok(res), | ||
| Err(e) => return Box::pin(future::err(e.into())), | ||
| }; | ||
|
|
||
| let res = res.map(|value| RpcResponse { | ||
| context: RpcResponseContext::new(state_reader.epoch_info.absolute_slot), | ||
| value, | ||
| }); | ||
|
|
||
| Box::pin(future::ready(res)) | ||
| let account = state_reader.svm.get_account(&pubkey); | ||
|
|
||
| // Drop the lock on the state while we fetch accounts | ||
| let absolute_slot = state_reader.epoch_info.absolute_slot; | ||
| let rpc_client = state_reader.rpc_client.clone(); | ||
| let encoding = config.encoding.clone(); | ||
| let data_slice = config.data_slice.clone(); | ||
| drop(state_reader); | ||
|
|
||
| Box::pin(async move { | ||
| let account = if let None = account { | ||
| // Fetch and save the missing account | ||
| if let Some(fetched_account) = rpc_client.get_account(&pubkey).await.ok() { | ||
| let mut state_reader = meta.get_state_mut()?; | ||
| state_reader | ||
| .svm | ||
| .set_account(pubkey, fetched_account.clone()) | ||
| .map_err(|err| { | ||
| Error::invalid_params(format!( | ||
| "failed to save fetched account {pubkey:?}: {err:?}" | ||
| )) | ||
| })?; | ||
|
|
||
| Some(fetched_account) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| account | ||
| }; | ||
|
|
||
| Ok(RpcResponse { | ||
| context: RpcResponseContext::new(absolute_slot), | ||
| value: account.map(|account| { | ||
| encode_ui_account( | ||
| &pubkey, | ||
| &account, | ||
| encoding.unwrap_or(UiAccountEncoding::Base64), | ||
| None, | ||
| data_slice, | ||
| ) | ||
| }), | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Async-based account retrieval logic looks solid but silently discards fetch errors.
- Dropping the lock before partially fetching prevents blocking other threads, which is good. However, in line 102 (
.ok()), you ignore theErrvariant from RPC. Consider at least logging the error or returning a more informative response to the client. - Parallel checks for missing accounts: if multiple callers fetch the same missing account simultaneously, you may get redundant RPC calls. A concurrency-aware memoization strategy could help prevent race conditions or repeated requests.
- Partial error vs. partial success: If the account retrieval fails, you return
Nonefor the account in yourRpcResponse. This is acceptable as a fallback but can be confusing to the caller. You might offer an explicit error field in the response.
Below is a suggested diff to handle the remote fetch error more explicitly (applicable to the relevant lines near 102):
- if let Some(fetched_account) = rpc_client.get_account(&pubkey).await.ok() {
+ let remote_fetch = rpc_client.get_account(&pubkey).await;
+ if let Ok(fetched_account) = remote_fetch {
let mut state_reader = meta.get_state_mut()?;
...
} else if let Err(e) = remote_fetch {
// Log or handle the specific error here
warn!("Failed to fetch account from remote: {:?}", e);
}📝 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.
| let config = config.unwrap_or_default(); | |
| let pubkey = match verify_pubkey(&pubkey_str) { | |
| Ok(res) => res, | |
| Err(e) => return Box::pin(future::err(e)), | |
| }; | |
| let config = config.unwrap_or_default(); | |
| let state_reader = match meta.get_state() { | |
| Ok(res) => res, | |
| Err(e) => return Box::pin(future::err(e.into())), | |
| }; | |
| let res = match transform_account_to_ui_account( | |
| &state_reader.svm.get_account(&pubkey), | |
| &config, | |
| ) { | |
| Ok(res) => Ok(res), | |
| Err(e) => return Box::pin(future::err(e.into())), | |
| }; | |
| let res = res.map(|value| RpcResponse { | |
| context: RpcResponseContext::new(state_reader.epoch_info.absolute_slot), | |
| value, | |
| }); | |
| Box::pin(future::ready(res)) | |
| let account = state_reader.svm.get_account(&pubkey); | |
| // Drop the lock on the state while we fetch accounts | |
| let absolute_slot = state_reader.epoch_info.absolute_slot; | |
| let rpc_client = state_reader.rpc_client.clone(); | |
| let encoding = config.encoding.clone(); | |
| let data_slice = config.data_slice.clone(); | |
| drop(state_reader); | |
| Box::pin(async move { | |
| let account = if let None = account { | |
| // Fetch and save the missing account | |
| if let Some(fetched_account) = rpc_client.get_account(&pubkey).await.ok() { | |
| let mut state_reader = meta.get_state_mut()?; | |
| state_reader | |
| .svm | |
| .set_account(pubkey, fetched_account.clone()) | |
| .map_err(|err| { | |
| Error::invalid_params(format!( | |
| "failed to save fetched account {pubkey:?}: {err:?}" | |
| )) | |
| })?; | |
| Some(fetched_account) | |
| } else { | |
| None | |
| } | |
| } else { | |
| account | |
| }; | |
| Ok(RpcResponse { | |
| context: RpcResponseContext::new(absolute_slot), | |
| value: account.map(|account| { | |
| encode_ui_account( | |
| &pubkey, | |
| &account, | |
| encoding.unwrap_or(UiAccountEncoding::Base64), | |
| None, | |
| data_slice, | |
| ) | |
| }), | |
| }) | |
| }) | |
| let config = config.unwrap_or_default(); | |
| let pubkey = match verify_pubkey(&pubkey_str) { | |
| Ok(res) => res, | |
| Err(e) => return Box::pin(future::err(e)), | |
| }; | |
| let state_reader = match meta.get_state() { | |
| Ok(res) => res, | |
| Err(e) => return Box::pin(future::err(e.into())), | |
| }; | |
| let account = state_reader.svm.get_account(&pubkey); | |
| // Drop the lock on the state while we fetch accounts | |
| let absolute_slot = state_reader.epoch_info.absolute_slot; | |
| let rpc_client = state_reader.rpc_client.clone(); | |
| let encoding = config.encoding.clone(); | |
| let data_slice = config.data_slice.clone(); | |
| drop(state_reader); | |
| Box::pin(async move { | |
| let account = if let None = account { | |
| // Fetch and save the missing account | |
| + let remote_fetch = rpc_client.get_account(&pubkey).await; | |
| + if let Ok(fetched_account) = remote_fetch { | |
| let mut state_reader = meta.get_state_mut()?; | |
| state_reader | |
| .svm | |
| .set_account(pubkey, fetched_account.clone()) | |
| .map_err(|err| { | |
| Error::invalid_params(format!( | |
| "failed to save fetched account {pubkey:?}: {err:?}" | |
| )) | |
| })?; | |
| + Some(fetched_account) | |
| + } else if let Err(e) = remote_fetch { | |
| + // Log or handle the specific error here | |
| + warn!("Failed to fetch account from remote: {:?}", e); | |
| + None | |
| + } else { | |
| + None | |
| } | |
| } else { | |
| account | |
| }; | |
| Ok(RpcResponse { | |
| context: RpcResponseContext::new(absolute_slot), | |
| value: account.map(|account| { | |
| encode_ui_account( | |
| &pubkey, | |
| &account, | |
| encoding.unwrap_or(UiAccountEncoding::Base64), | |
| None, | |
| data_slice, | |
| ) | |
| }), | |
| }) | |
| }) |
|
Thanks so much @Dodecahedr0x! |
Summary by CodeRabbit
Refactor
New Features