rpcserver: Support named and null parameters, Review optionals on rpc internal methods, Stronger Response and Error codes by jaoleal · Pull Request #831 · getfloresta/Floresta

jaoleal · 2026-02-06T20:51:16Z

Description and Notes

Some time ago i was messing with the server trying to fix some pains that i had developing methods on it and i encountered some points to upgrade.

4839dc6 - fixes #827 by matching errors and returning constant values, i also mapped some custom error codes to internal errors as I understood that were allowed by the spec.

29220da- Adds named and null parameters support, I also reviewed some Options in internal rpc methods that actually had internal defaults, created a getter for that case.

fix #827
fix #704

How to verify the changes you have done?

Pushed some extensive tests regarding request and result parsing and behavior, checking that calls are accepted when expected and when refused we assert the error response.

Contributor Checklist

I've followed the contribution guidelines
I've verified one of the following:
- Ran just pcc (recommended but slower)
- Ran just lint-features '-- -D warnings' && cargo test --release
- Confirmed CI passed on my fork
I've linked any related issue(s) in the sections above

Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).

jaoleal · 2026-02-06T20:53:48Z

cc @csgui

Why i cant alter the name of the PR 😭

jaoleal · 2026-02-06T23:28:05Z

Can someone with permission to alter the pr title fix it for me ?

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

jaoleal · 2026-02-06T23:33:55Z

5cd9007 Unrelated test fail

csgui · 2026-02-08T20:05:16Z

@jaoleal double check if you can use <T: DeserializeOwned> or <'de, T: Deserialize<'de>>. Not both, choose one.

That would makes more sense and allows the remove of clone in the code. When parsing RPC parameters clone can cause performance issues and increased memory pressure under heavy load. We do not have control on what Floresta user is passing to a RPC call.

jaoleal · 2026-02-09T09:25:48Z

top commit: 0e160f5

Removed a clone from get_at, lifetime matching solved that.
Included back MempoolAccept, sorry conflict problems.
Fixed a bug matching against params in get_arr_at where it would result unwrapping in a malformed input.
Added extensive tests, they were written by claudiao but i reviewed it and found a bug.
Fixed a bug when an optional parameter is omitted when params are omitted as a whole.

Thanks for the suggestions @csgui

moisesPompilio · 2026-02-09T13:43:13Z

I think the commit descriptions are missing because they only have titles.

jaoleal · 2026-02-09T19:08:38Z

[781ea91 Rebased and extended commit message with descriptions.

jaoleal · 2026-02-09T19:39:32Z

Added formal fix statement for the issues that this pr fixes.

jaoleal · 2026-02-10T18:17:14Z

gaddamit these conflicts

jaoleal · 2026-02-10T21:17:49Z

unrelated fail on functional tests d18e6dd

jaoleal · 2026-03-12T17:07:05Z

@moisesPompilio done in 0f62bfa

jaoleal · 2026-03-16T17:39:09Z

rebased

moisesPompilio · 2026-03-17T20:48:51Z

+//! [`RpcError`]: jsonrpc_interface::RpcError
+//! [`JsonRpcError`]: jsonrpc_interface::JsonRpcError
+
+use std::fmt::Debug;


e8938d4

use core instead std

moisesPompilio · 2026-03-17T20:49:00Z

+    use std::fmt::Display;
+    use std::fmt::Formatter;


e8938d4

use core instead std

moisesPompilio · 2026-03-17T20:54:36Z

need rebase

- Add named constants for floresta-specific rpc server error codes (TX_NOT_FOUND, BLOCK_NOT_FOUND, PEER_NOT_FOUND, etc.) - Add SERIALIZATION_EXPECT and HTTP_RESPONSE_EXPECT constants for expect() messages - Add From<BlockchainError> for JsonRpcError conversion. - Change ScriptPubKeyJson.address to Option<String> for nonstandard scripts (matching Bitcoin Core behavior)

- Rewrite arg_parser with organic get_at(), get_with_default(), get_arr_at(), and optional(). - Change RpcRequest.params to support named (object) parameters alongside positional (array) parameters. - Split method dispatch into no-param methods (early return) and param methods (with params.unwrap_or_default()). - Replace .unwrap() calls with .expect() using descriptive constants. - Build explicit HTTP responses for parse errors with proper status codes.

…d clippy lint - Replace all .unwrap() calls in blockchain.rs with proper error propagation via map_err and the ? operator - Apply #[deny(clippy::unwrap_used)] lint to the json_rpc module

- Add comprehensive tests covering positional and named parameters, null/omitted params, optional defaults, error codes, and response structure validation

csgui · 2026-03-20T18:22:10Z

@jaoleal

Double-checking this PR, and it seems there are still some things to improve.

1 - Run the command below and check the response:

command: curl -i --data-binary '{"jsonrpc":"1.0","id":"xyz","method":"getblock","params":42}' -H 'content-type: application/json;' http://127.0.0.1:8332

response: {"result":null,"error":{"code":-32602,"message":"Missing parameter","data":"block_hash"},"id":"xyz"}

The correct response would be something like InvalidParameterType. params is a number. The server accepted the malformed param and is blaming a missing field instead of identifying the actual problem.

2 - Run the command below and check the response:

command: curl -i --data-binary '{"jsonrpc":"1.0","id":"xyz","method":"getblock","params":null}' -H 'content-type: application/json;' http://127.0.0.1:8332

response: {"result":null,"error":{"code":-32602,"message":"Missing parameter","data":"block_hash"},"id":"xyz"}

It is the same MissingParameter instead of the server recognizing that getblock requires params and rejects the request.

Also, corner cases like those, should be covered by some kind of test.

jaoleal · 2026-03-20T18:40:23Z

Thanks for finding these bugs @csgui, ill investigate them and address a fix.

By intuition a fix would be a general check done on each request, as it is now problems are found by the command execution pipeline.

Also, corner cases like those, should be covered by some kind of test.

Ill be more aggressive on these

vatsalkeshav · 2026-04-11T08:01:39Z

I think this is also related to #463 scoping floresta_node/s/json_rpc/

moisesPompilio · 2026-04-13T14:30:31Z


+/// Types and methods implementing the [JSON-RPC 2.0 spec](https://www.jsonrpc.org/specification),
+/// tailored for floresta's RPC server.
+pub mod jsonrpc_interface {


NIT:
It doesn't seem good to have a mod inside a file. Maybe it's interesting to put this content inside that mod json_interface into a new file, or make res.rs a folder res/ and what is inside it become a mod, and what is inside that json_interface mod would go to a new file inside it calledjson_interface.rsor something like that. I think it looks nicer

No yeah, totally agree with you, we actually reimplement a lot of things from jsonrpc (rust-bitcoins lib) and expand some others like the getters we have.

I wish we centralized those to avoid code duplication and drifting which still happening thanks to us not agreeing with an approach, I may give it another shot on Q3 but right now im focusing in delivering the most rpcs i can until there.

jaoleal · 2026-04-13T15:03:13Z

I think this is also related to #463 scoping floresta_node/s/json_rpc/

Yes, it is!

jaoleal · 2026-04-13T15:13:50Z

Also guys, I want to refact the tests for this PR as it seems to not cover all those cases where the rpc interface can fail, but for that i really wish for #794 to be merged before to void more rework...

Unique-Usman · 2026-04-13T22:19:36Z

@@ -19,7 +19,7 @@ pub struct RpcRequest {
    pub method: String,

    /// The parameters for the method, as an array of json values.


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

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.

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

Davidson-Souza · 2026-04-13T23:00:06Z

Needs rebase.

Also guys, I want to refact the tests for this PR as it seems to not cover all those cases where the rpc interface can fail, but for that i really wish for #794 to be merged before to void more rework...

Since this depends on #794 and will go through some major changes, I recommend changing back to draft.

jaoleal · 2026-04-13T23:11:00Z

done!

jaoleal force-pushed the some_fixes_rpcserver branch from 766648c to 5cd9007 Compare February 6, 2026 23:25

Davidson-Souza assigned jaoleal Feb 7, 2026

Davidson-Souza added RPC Changes something with our JSON-RPC interface code quality Generally improves code readability and maintainability labels Feb 7, 2026

Davidson-Souza changed the title ~~rpcserver: Support named and null parameters, Review optionals on rpc internal methods,~~ rpcserver: Support named and null parameters, Review optionals on rpc internal methods, Stronger Response and Error codes Feb 7, 2026

csgui requested changes Feb 7, 2026

View reviewed changes

Comment thread crates/floresta-node/src/json_rpc/request.rs Outdated

csgui requested changes Feb 7, 2026

View reviewed changes

Comment thread crates/floresta-node/src/json_rpc/request.rs Outdated

Comment thread crates/floresta-node/src/json_rpc/request.rs Outdated

Comment thread crates/floresta-node/src/json_rpc/request.rs Outdated

Comment thread crates/floresta-node/src/json_rpc/request.rs Outdated

jaoleal force-pushed the some_fixes_rpcserver branch 2 times, most recently from c13b68a to 0e160f5 Compare February 9, 2026 09:22

jaoleal requested review from Davidson-Souza and moisesPompilio February 9, 2026 09:36

jaoleal force-pushed the some_fixes_rpcserver branch from 0e160f5 to 781ea91 Compare February 9, 2026 19:07

jaoleal force-pushed the some_fixes_rpcserver branch from 781ea91 to 994e4b7 Compare February 10, 2026 01:44

This comment was marked as outdated.

Sign in to view

jaoleal force-pushed the some_fixes_rpcserver branch 4 times, most recently from 2d230a8 to bd4d7a1 Compare February 10, 2026 15:45

jaoleal force-pushed the some_fixes_rpcserver branch from bd4d7a1 to d18e6dd Compare February 10, 2026 20:32

jaoleal requested a review from csgui February 11, 2026 18:07

csgui removed the RPC Changes something with our JSON-RPC interface label Mar 11, 2026

csgui added this to the Q2/2026 milestone Mar 11, 2026

moisesPompilio reviewed Mar 12, 2026

View reviewed changes

jaoleal force-pushed the some_fixes_rpcserver branch from f8aa0e3 to 0f62bfa Compare March 12, 2026 17:06

jaoleal force-pushed the some_fixes_rpcserver branch 2 times, most recently from edad67c to d4afa59 Compare March 16, 2026 17:37

moisesPompilio reviewed Mar 17, 2026

View reviewed changes

jaoleal added 4 commits March 18, 2026 03:10

fix(rpcserver): Remove unwrap() calls from blockchain handlers and ad…

bb52e4b

…d clippy lint - Replace all .unwrap() calls in blockchain.rs with proper error propagation via map_err and the ? operator - Apply #[deny(clippy::unwrap_used)] lint to the json_rpc module

test(rpcserver): Integration tests for JSON-RPC request parsing

8ecf173

- Add comprehensive tests covering positional and named parameters, null/omitted params, optional defaults, error codes, and response structure validation

jaoleal force-pushed the some_fixes_rpcserver branch from d4afa59 to 8ecf173 Compare March 18, 2026 06:15

jaoleal added RPC Changes something with our JSON-RPC interface architecture labels Mar 27, 2026

moisesPompilio reviewed Apr 13, 2026

View reviewed changes

Unique-Usman reviewed Apr 13, 2026

View reviewed changes

jaoleal marked this pull request as draft April 13, 2026 23:10

This was referenced Apr 14, 2026

Network RPCs: getaddednodeinfo, addpeeraddress, getnodedaddresses e getaddrmaninfo. #916

Draft

Fix a bug in invalidateblock. Add Test rpcs, invalidateblock and submitheader. #881

Draft

Refactor(electrum): Remove unwraps and expects #956

Open

		@@ -19,7 +19,7 @@ pub struct RpcRequest {
		pub method: String,

		/// The parameters for the method, as an array of json values.

Conversation

jaoleal commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and Notes

How to verify the changes you have done?

Contributor Checklist

Uh oh!

jaoleal commented Feb 6, 2026

Uh oh!

jaoleal commented Feb 6, 2026

Uh oh!

jaoleal commented Feb 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csgui commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaoleal commented Feb 9, 2026

Uh oh!

moisesPompilio commented Feb 9, 2026

Uh oh!

jaoleal commented Feb 9, 2026

Uh oh!

jaoleal commented Feb 9, 2026

Uh oh!

This comment was marked as outdated.

jaoleal commented Feb 10, 2026

Uh oh!

jaoleal commented Feb 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Mar 12, 2026

Uh oh!

jaoleal commented Mar 16, 2026

Uh oh!

moisesPompilio Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

moisesPompilio Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

moisesPompilio commented Mar 17, 2026

Uh oh!

csgui commented Mar 20, 2026

Uh oh!

jaoleal commented Mar 20, 2026

Uh oh!

vatsalkeshav commented Apr 11, 2026

Uh oh!

moisesPompilio Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

jaoleal Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

jaoleal commented Apr 13, 2026

Uh oh!

jaoleal commented Apr 13, 2026

Uh oh!

Unique-Usman Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

jaoleal Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Unique-Usman Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Davidson-Souza commented Apr 13, 2026

jaoleal commented Feb 6, 2026 •

edited

Loading

csgui commented Feb 8, 2026 •

edited

Loading