Skip to content

rpcserver: Support named and null parameters, Review optionals on rpc internal methods, Stronger Response and Error codes #831

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.

Sign up for GitHub

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

Draft
jaoleal wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

rpcserver: Support named and null parameters, Review optionals on rpc internal methods, Stronger Response and Error codes #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4411380
fix(rpcserver): Improve error types and response handling
jaoleal Feb 5, 2026
acae167
fix(rpcserver): Refactor arg_parser and support named/null parameters
jaoleal Feb 24, 2026
bb52e4b
fix(rpcserver): Remove unwrap() calls from blockchain handlers and ad…
jaoleal Feb 24, 2026
8ecf173
test(rpcserver): Integration tests for JSON-RPC request parsing
jaoleal Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 38 additions & 19 deletions crates/floresta-node/src/json_rpc/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,22 @@ use serde_json::json;
use serde_json::Value;
use tracing::debug;

use super::res::jsonrpc_interface::JsonRpcError;
use super::res::GetBlockchainInfoRes;
use super::res::GetTxOutProof;
use super::res::JsonRpcError;
use super::server::RpcChain;
use super::server::RpcImpl;
use crate::json_rpc::res::GetBlockRes;
use crate::json_rpc::res::RescanConfidence;
use crate::json_rpc::server::SERIALIZATION_EXPECT;

impl<Blockchain: RpcChain> RpcImpl<Blockchain> {
async fn get_block_inner(&self, hash: BlockHash) -> Result<Block, JsonRpcError> {
let is_genesis = self.chain.get_block_hash(0).unwrap().eq(&hash);
let is_genesis = self
.chain
.get_block_hash(0)
.map_err(|_| JsonRpcError::Chain)?
.eq(&hash);

if is_genesis {
return Ok(genesis_block(self.network));
Expand All @@ -53,7 +58,11 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {
.wallet
.get_height(txid)
.ok_or(JsonRpcError::TxNotFound)?;
let blockhash = self.chain.get_block_hash(height).unwrap();
let blockhash = self
.chain
.get_block_hash(height)
.map_err(|_| JsonRpcError::BlockNotFound)?;

self.chain
.get_block(&blockhash)
.map_err(|_| JsonRpcError::BlockNotFound)
Expand All @@ -62,17 +71,11 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {
pub fn get_rescan_interval(
&self,
use_timestamp: bool,
start: Option<u32>,
stop: Option<u32>,
confidence: Option<RescanConfidence>,
start: u32,
stop: u32,
confidence: RescanConfidence,
) -> Result<(u32, u32), JsonRpcError> {
let start = start.unwrap_or(0u32);
let stop = stop.unwrap_or(0u32);

if use_timestamp {
let confidence = confidence.unwrap_or(RescanConfidence::Medium);
// `get_block_height_by_timestamp` already does the time validity checks.

let start_height = self.get_block_height_by_timestamp(start, &confidence)?;

let stop_height = self.get_block_height_by_timestamp(stop, &RescanConfidence::Exact)?;
Expand Down Expand Up @@ -165,7 +168,11 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {

// getbestblockhash
pub(super) fn get_best_block_hash(&self) -> Result<BlockHash, JsonRpcError> {
Ok(self.chain.get_best_block().unwrap().1)
Ok(self
.chain
.get_best_block()
.map_err(|_| JsonRpcError::Chain)?
.1)
}

// getblock
Expand Down Expand Up @@ -244,10 +251,19 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {

// getblockchaininfo
pub(super) fn get_blockchain_info(&self) -> Result<GetBlockchainInfoRes, JsonRpcError> {
let (height, hash) = self.chain.get_best_block().unwrap();
let validated = self.chain.get_validation_index().unwrap();
let (height, hash) = self
.chain
.get_best_block()
.map_err(|_| JsonRpcError::Chain)?;
Copy link
Copy Markdown
Member

@Davidson-Souza Davidson-Souza Mar 3, 2026

Choose a reason for hiding this comment

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

Damn need a concrete error type for chain ASAP, this sucks

Copy link
Copy Markdown
Member Author

@jaoleal jaoleal Mar 4, 2026

Choose a reason for hiding this comment

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

I can do that

Copy link
Copy Markdown
Member

@Davidson-Souza Davidson-Souza Mar 4, 2026

Choose a reason for hiding this comment

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

I think it should be in a follow-up, this PR is already pretty big. But sure, be my guest to take a shot on it

Copy link
Copy Markdown
Member Author

@jaoleal jaoleal Mar 5, 2026

Choose a reason for hiding this comment

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

Okay, follow up pr.

Do you have any approach in mind ?

let validated = self
.chain
.get_validation_index()
.map_err(|_| JsonRpcError::Chain)?;
let ibd = self.chain.is_in_ibd();
let latest_header = self.chain.get_block_header(&hash).unwrap();
let latest_header = self
.chain
.get_block_header(&hash)
.map_err(|_| JsonRpcError::Chain)?;
let latest_work = latest_header
.calculate_chain_work(&self.chain)?
.to_string_hex();
Expand All @@ -262,7 +278,10 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {
.map(|r| r.to_string())
.collect();

let validated_blocks = self.chain.get_validation_index().unwrap();
let validated_blocks = self
.chain
.get_validation_index()
.map_err(|_| JsonRpcError::Chain)?;

let validated_percentage = if height != 0 {
validated_blocks as f32 / height as f32
Expand All @@ -288,7 +307,7 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {

// getblockcount
pub(super) fn get_block_count(&self) -> Result<u32, JsonRpcError> {
Ok(self.chain.get_height().unwrap())
self.chain.get_height().map_err(|_| JsonRpcError::Chain)
}

// getblockfilter
Expand Down Expand Up @@ -600,7 +619,7 @@ impl<Blockchain: RpcChain> RpcImpl<Blockchain> {
height: u32,
) -> Result<Value, JsonRpcError> {
if let Some(txout) = self.wallet.get_utxo(&OutPoint { txid, vout }) {
return Ok(serde_json::to_value(txout).unwrap());
return Ok(serde_json::to_value(txout).expect(SERIALIZATION_EXPECT));
}

// if we are on IBD, we don't have any filters to find this txout.
Expand Down
2 changes: 1 addition & 1 deletion crates/floresta-node/src/json_rpc/control.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use serde::Deserialize;
use serde::Serialize;

use super::res::JsonRpcError;
use super::res::jsonrpc_interface::JsonRpcError;
use super::server::RpcChain;
use super::server::RpcImpl;

Expand Down
2 changes: 1 addition & 1 deletion crates/floresta-node/src/json_rpc/network.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use floresta_wire::node_interface::PeerInfo;
use serde_json::json;
use serde_json::Value;

use super::res::JsonRpcError;
use super::res::jsonrpc_interface::JsonRpcError;
use super::server::RpcChain;
use super::server::RpcImpl;

Expand Down
151 changes: 41 additions & 110 deletions crates/floresta-node/src/json_rpc/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub struct RpcRequest {
pub method: String,

/// The parameters for the method, as an array of json values.
Copy link
Copy Markdown
Contributor

@Unique-Usman Unique-Usman Apr 13, 2026

Choose a reason for hiding this comment

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

nit: I think the comment should be updated, maybe "an optional array of json values" ?

Copy link
Copy Markdown
Member Author

@jaoleal jaoleal Apr 13, 2026

Choose a reason for hiding this comment

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

Yes, nice catch, regarding the new doc; the whole parameter should be a Json value by itself because we expect both an array and an object.

Copy link
Copy Markdown
Contributor

@Unique-Usman Unique-Usman Apr 13, 2026

Choose a reason for hiding this comment

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

that then make the whole statement of it being only array of json values wrong. Maybe sonething around "JSON value (array or object)"

pub params: Vec<Value>,
pub params: Option<Value>,

/// An optional identifier for the request, which can be used to match responses.
pub id: Value,
Expand All @@ -29,130 +29,61 @@ pub struct RpcRequest {
/// methods already handle the case where the parameter is missing or has an
/// unexpected type, returning an error if so.
pub mod arg_parser {
use core::str::FromStr;

use serde::Deserialize;
use serde_json::Value;

use crate::json_rpc::res::JsonRpcError;
use crate::json_rpc::res::jsonrpc_interface::JsonRpcError;

/// Extracts a u64 parameter from the request parameters at the specified index.
///
/// This function checks if the parameter exists, is of type u64 and can be converted to `T`.
/// Returns an error otherwise.
pub fn get_numeric<T: TryFrom<u64>>(
params: &[Value],
index: usize,
opt_name: &str,
) -> Result<T, JsonRpcError> {
let v = params
.get(index)
.ok_or_else(|| JsonRpcError::MissingParameter(opt_name.to_string()))?;

let n = v.as_u64().ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be a number"))
})?;

T::try_from(n)
.map_err(|_| JsonRpcError::InvalidParameterType(format!("{opt_name} is out-of-range")))
}

/// Extracts a string parameter from the request parameters at the specified index.
///
/// This function checks if the parameter exists and is of type string. Returns an error
/// otherwise.
pub fn get_string(
params: &[Value],
index: usize,
opt_name: &str,
) -> Result<String, JsonRpcError> {
let v = params
.get(index)
.ok_or_else(|| JsonRpcError::MissingParameter(opt_name.to_string()))?;

let str = v.as_str().ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be a string"))
})?;

Ok(str.to_string())
}

/// Extracts a boolean parameter from the request parameters at the specified index.
///
/// This function checks if the parameter exists and is of type boolean. Returns an error
/// otherwise.
pub fn get_bool(params: &[Value], index: usize, opt_name: &str) -> Result<bool, JsonRpcError> {
let v = params
.get(index)
.ok_or_else(|| JsonRpcError::MissingParameter(opt_name.to_string()))?;

v.as_bool().ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be a boolean"))
})
}

/// Extracts a hash parameter from the request parameters at the specified index.
/// Extracts a parameter from the request parameters at the specified index.
///
/// This function can extract any type that implements `FromStr`, such as `BlockHash` or
/// `Txid`. It checks if the parameter exists and is a valid string representation of the type.
/// Returns an error otherwise.
pub fn get_hash<T: FromStr>(
params: &[Value],
pub fn get_at<'de, T: Deserialize<'de>>(
params: &'de Value,
index: usize,
opt_name: &str,
field_name: &str,
) -> Result<T, JsonRpcError> {
let v = params
.get(index)
.ok_or_else(|| JsonRpcError::MissingParameter(opt_name.to_string()))?;

v.as_str().and_then(|s| s.parse().ok()).ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be a valid hash"))
})
let v = match (params.is_array(), params.is_object()) {
(true, false) => params.get(index),
(false, true) => params.get(field_name),
_ => None,
};
Comment thread
jaoleal marked this conversation as resolved.

let unwrap = v.ok_or(JsonRpcError::MissingParameter(field_name.to_string()))?;

T::deserialize(unwrap)
.ok()
.ok_or(JsonRpcError::InvalidParameterType(format!(
"{field_name} has an invalid type"
)))
}

/// Extracts an array of hashes from the request parameters at the specified index.
///
/// This function can extract an array of any type that implements `FromStr`, such as
/// `BlockHash` or `Txid`. It checks if the parameter exists and is an array of valid string
/// representations of the type. Returns an error otherwise.
pub fn get_hashes_array<T: FromStr>(
params: &[Value],
index: usize,
opt_name: &str,
) -> Result<Vec<T>, JsonRpcError> {
let v = params
.get(index)
.ok_or_else(|| JsonRpcError::MissingParameter(opt_name.to_string()))?;

let array = v.as_array().ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be an array of hashes"))
})?;

array
.iter()
.map(|v| {
v.as_str().and_then(|s| s.parse().ok()).ok_or_else(|| {
JsonRpcError::InvalidParameterType(format!("{opt_name} must be a valid hash"))
})
})
.collect()
/// Wraps a parameter extraction result so that a missing parameter yields `Ok(None)`
/// instead of an error. Other errors are propagated unchanged.
pub fn try_into_optional<T>(
result: Result<T, JsonRpcError>,
) -> Result<Option<T>, JsonRpcError> {
match result {
Ok(t) => Ok(Some(t)),
Err(JsonRpcError::MissingParameter(_)) => Ok(None),
Err(e) => Err(e),
}
}

/// Extracts an optional field from the request parameters at the specified index.
///
/// This function checks if the parameter exists and is of the expected type. If the parameter
/// doesn't exist, it returns `None`. If it exists but is of an unexpected type, it returns an
/// error.
pub fn get_optional_field<T>(
params: &[Value],
/// Like [`get_at`], but returns `default` when the parameter is missing instead of
/// an error. Type mismatches are still propagated as errors.
pub fn get_with_default<'de, T: Deserialize<'de>>(
v: &'de Value,
index: usize,
opt_name: &str,
extractor_fn: impl Fn(&[Value], usize, &str) -> Result<T, JsonRpcError>,
) -> Result<Option<T>, JsonRpcError> {
if params.len() <= index {
return Ok(None);
field_name: &str,
default: T,
) -> Result<T, JsonRpcError> {
match get_at(v, index, field_name) {
Ok(t) => Ok(t),
Err(JsonRpcError::MissingParameter(_)) => Ok(default),
Err(e) => Err(e),
}

let value = extractor_fn(params, index, opt_name)?;
Ok(Some(value))
}
}
Loading
Loading