getBalance implementation #12
Conversation
WalkthroughThe changes refactor the RPC interface by removing the existing Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/core/src/rpc/accounts_data.rs (1)
144-165: Consider optimizing the balance retrieval implementation.The current implementation has several areas for improvement:
- Error handling could be more specific rather than using
map_or(0, |account| account.lamports)RpcAccountInfoConfig::default()creates an unnecessary allocation- Fetching full account data when only lamports are needed may impact performance
Consider this optimized implementation:
fn get_balance( &self, meta: Self::Metadata, pubkey_str: String, _config: Option<RpcContextConfig>, ) -> BoxFuture<Result<RpcResponse<u64>>> { let absolute_slot = match meta.get_state() { Ok(s) => s.epoch_info.absolute_slot, Err(e) => return Box::pin(future::err(e.into())), }; - let account = - self.get_account_info(meta, pubkey_str, Some(RpcAccountInfoConfig::default())); + let pubkey = match verify_pubkey(&pubkey_str) { + Ok(pk) => pk, + 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 lamports = state_reader.svm.get_account(&pubkey) + .map(|acc| acc.lamports) + .unwrap_or(0); + + let rpc_client = state_reader.rpc_client.clone(); + drop(state_reader); Box::pin(async move { - let lamports = account.await?.value.map_or(0, |account| account.lamports); + let lamports = if lamports == 0 { + // Only fetch from RPC if not found locally + rpc_client.get_account(&pubkey).await + .map(|acc| acc.lamports) + .unwrap_or(0) + } else { + lamports + }; Ok(RpcResponse { context: RpcResponseContext::new(absolute_slot), value: lamports, }) }) }This optimization:
- Avoids fetching full account data when only lamports are needed
- Provides clearer error handling
- Reduces unnecessary allocations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/core/src/rpc/accounts_data.rs(4 hunks)crates/core/src/rpc/minimal.rs(0 hunks)
💤 Files with no reviewable changes (1)
- crates/core/src/rpc/minimal.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_cargo_checks
- GitHub Check: build
🔇 Additional comments (2)
crates/core/src/rpc/accounts_data.rs (2)
12-12: LGTM!The addition of
RpcContextConfigimport is appropriate for the new balance retrieval functionality.
33-39: LGTM!The
get_balancetrait method is well-defined with appropriate parameters and return type. The use ofRpcContextConfigallows for future extensibility.
crates/core/src/rpc/minimal.rs
Outdated
| @@ -23,14 +23,6 @@ use solana_sdk::{ | |||
| pub trait Minimal { | |||
| type Metadata; | |||
|
|
|||
| #[rpc(meta, name = "getBalance")] | |||
| fn get_balance( | |||
There was a problem hiding this comment.
The shape of these traits are coming from the canonical implementation.
I personally don't necessarily agree with the way they've been designed, but I think staying consistent with the canonical design would help us maintaining this set of methods.
Could we please keep the get_balance implementation in the Minimal trait?
There was a problem hiding this comment.
Sure, I wasn't aware of the canonical design and did that because I wanted to reuse the implementation from accounts data. Ideally there should be a trait implemented by the run loop that fetches and updates local accounts if they are missing but I was facing ownership issues.
Could you point me to the official design please?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
59-63: Fix markdown list indentation.The list items have inconsistent indentation. They should be indented by 4 spaces to match the parent list's style.
- - [x] getMinimumBalanceForRentExemption - - [x] getBalance - [ ] simulateTransaction - [ ] getTransaction - - [x] requestAirdrop + - [x] getMinimumBalanceForRentExemption + - [x] getBalance - [ ] simulateTransaction - [ ] getTransaction + - [x] requestAirdrop🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
59-59: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
60-60: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)crates/core/src/rpc/accounts_data.rs(1 hunks)crates/core/src/rpc/minimal.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/core/src/rpc/accounts_data.rs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
59-59: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
60-60: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: run_cargo_checks
🔇 Additional comments (2)
crates/core/src/rpc/minimal.rs (2)
27-32: LGTM! The async conversion looks good.The change to return
BoxFutureis appropriate for potentially long-running operations like account fetching.Consider documenting why the
configparameter is currently unused:- _config: Option<RpcContextConfig>, + _config: Option<RpcContextConfig>, // TODO: implement commitment level support✅ Verification successful
Async conversion and parameter documentation are appropriately handled.
- The conversion to a
BoxFutureis suitable for long-running operations.- The underscore prefix on
_configcorrectly indicates that the parameter is currently unused.- As suggested, adding a comment (e.g.,
// TODO: implement commitment level support) to explain why_configis unused would improve clarity.
97-146: Verify error handling in account fetching logic.The account fetching logic might silently ignore RPC errors by using
.ok(). Consider propagating these errors for better error handling.- if let Some(fetched_account) = rpc_client.get_account(&pubkey).await.ok() { + match rpc_client.get_account(&pubkey).await { + Ok(fetched_account) => { 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 { + } + Err(err) => { + return Err(Error::invalid_params(format!( + "failed to fetch account {pubkey:?}: {err:?}" + ))); + } + }
Uses
getAccountInfoto potentially reuse the account fetching logicSummary by CodeRabbit
New Features
getMinimumBalanceForRentExemption,getBalance, andrequestAirdrop.Refactor
get_block_commitmentmethod.Chores
get_block_commitmentmethod to streamline the interface.