[REFACTOR][RPC] Centralize RPC traits and method names#1067
[REFACTOR][RPC] Centralize RPC traits and method names#1067moisesPompilio wants to merge 7 commits into
Conversation
ed4a6ff to
2443380
Compare
c357d5a to
167b5c2
Compare
|
167b5c2 I rebased |
| self.call("loaddescriptor", &[Value::String(descriptor)]) | ||
| } | ||
|
|
||
| fn get_block_filter(&self, height: u32) -> Result<String> { |
There was a problem hiding this comment.
You removed get_block_filter?
There was a problem hiding this comment.
Yes, get_block_filter exists only in floresta-rpc, but it is not implemented in floresta-node
| @@ -0,0 +1,156 @@ | |||
| use core::fmt::Debug; | |||
There was a problem hiding this comment.
SPDX headers and module-level docs
| use core::fmt::Debug; | ||
| use std::str::FromStr; | ||
|
|
||
| macro_rules! define_rpc_methods { |
There was a problem hiding this comment.
Docs for this macro would be nice!
| Methods::GetBlockCount => serde_json::to_string_pretty(&client.get_block_count()?)?, | ||
| Methods::GetTxOut { txid, vout } => { | ||
| serde_json::to_string_pretty(&client.get_tx_out(txid, vout)?)? | ||
| serde_json::to_string_pretty(&client.get_tx_out(txid, vout, false)?)? |
There was a problem hiding this comment.
In Bitcoin Core, get_tx_out has a third argument, include_mempool. Since we haven’t fully implemented the mempool, that value doesn’t really make much difference in our case, but I set it to false to reflect how the option would be handled in Bitcoin Core
There was a problem hiding this comment.
Could yout add a TODO mentioning this?
| /// Returns the transaction, assuming it is cached by our watch only wallet | ||
| #[command(name = "gettransaction")] | ||
| GetTransaction { txid: Txid, verbose: Option<bool> }, | ||
| GetTransaction { txid: Txid, verbose: Option<u32> }, |
| script: String, | ||
| height_hint: u32, | ||
| ) -> Result<Value> { | ||
| height: u32, |
| if result.is_null() { | ||
| return Err(Error::TxOutNotFound); | ||
| } |
There was a problem hiding this comment.
Is this right? Shouldn't this be handled at server side?
There was a problem hiding this comment.
This shouldn’t return an error. The result is an Option, so I changed it so that when the value is null, it returns None instead of an error.
| #!/usr/bin/env bash | ||
| set -e | ||
| CRATES=$(cargo metadata --format-version 1 --no-deps | jq -r '.packages[] | select(.source == null and (.targets | map(.kind | contains(["lib"])) | any)) | .name' | tr '\n' ' ') | ||
| echo "Building docs for crates: $CRATES" | ||
| for crate in $CRATES; do | ||
| echo "Running cargo doc for: $crate" | ||
| RUSTDOCFLAGS="--cfg docsrs -D warnings" cargo +nightly doc -p "$crate" --no-deps --lib --all-features --document-private-items || exit 1 | ||
| done |
There was a problem hiding this comment.
Hmm, we need to make sure we can build docs for https://docs.gefloresta.sh
There was a problem hiding this comment.
What specific error is happening in this case? In my tests, it is being generated correctly and I’m able to access it.
Replace hardcoded method name strings with a single source of truth. The RpcMethods enum provides FromStr, to_string(), and as_str() for type-safe method name handling, preventing divergence across codebase.
167b5c2 to
f163a85
Compare
Split RPC methods into category-specific traits (Blockchain, Wallet, Network, RawTransaction, Control) for better code organization, segregation, and flexible implementation across RPC and CLI.
Introduce maybe_async procedural macro to NetworkRpc trait to support both async and sync implementations. This allows: - Shared trait definitions between floresta-node (async) and floresta-rpc (sync) - Consistent interface across different implementations - NetworkRpc trait now implemented in floresta-node to ensure network operations are compatible between floresta-node and floresta-rpc Fix build compatibility issue: - maybe_async feature conflicts when building entire workspace - Adjusted justfile to compile individual crates by path instead of workspace super-set to prevent async feature leaking into floresta-cli - floresta-cli now builds without async feature to preserve RPC implementation
Add maybe_async support to WalletRpc trait enabling async/sync flexibility. WalletRpc now implemented in floresta-node to ensure consistency.
…a-node Enable async/sync flexibility for ControlRpc trait with maybe_async. Provides unified interface for control operations (get_memory_info, get_rpc_info, stop, uptime) across floresta-node and floresta-rpc.
…esta-node Enable async/sync flexibility for BlockchainRpc trait with maybe_async. Provides unified interface for blockchain operations across floresta-node and floresta-rpc, completing the RPC trait migration pattern.
Enable async/sync flexibility for RawTransactionRpc trait with maybe_async. Remove corepc_types dependency from floresta-node, centralizing all RPC types in floresta-rpc as single source of truth.
f163a85 to
c5b471f
Compare
Description and Notes
The RPC trait and method definitions were centralized so that
floresta-rpcandfloresta-nodestay aligned and compatible.At the moment,
floresta-rpcimplements RPC methods that are consumed by the node, whilefloresta-nodeexposes the server-side implementation that serves the same methods. To avoid inconsistencies between both crates, this change introduces shared RPC traits that define the contract for these methods in a single place.The main challenge is that
floresta-rpcneeds synchronous traits, whilefloresta-nodeneeds asynchronous ones. To solve this, this change introduces themaybe-asyncmacro, which allows the same trait definitions to support both sync and async implementations through a single code path.In addition to the trait unification, the RPC macros were reorganized into categories similar to Bitcoin Core:
walletblockchaincontrolnetworkutilrawtransactionsIt also introduces
RpcMethods, an enum that acts as the single source of truth for all RPC method names. It providesto_string(),as_str(), andFromStrimplementations so bothfloresta-rpcandfloresta-nodecan use the same method definitions safely. This prevents spelling mismatches and avoids duplicating method names across the codebase.How to verify the changes you have done?
floresta-rpcandfloresta-nodeto confirm the shared RPC traits compile correctly in both sync and async contexts.RpcMethodscan be converted withto_string(),as_str(), andFromStr.walletblockchaincontrolnetworkutilrawtransactionsmaybe-asyncintegration and the crate-by-crate build adjustment.