feat: batch fetch substates api - #1547
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. WalkthroughAdds a batched substates API end-to-end: JSON-RPC server/handler, client RPC methods/types, storage trait/SQLite implementation, and SubstateManager batch retrieval; plus supporting type, TS binding, walletd, UI, and error/deserialize adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RPC as Indexer JSON-RPC Server
participant H as JsonRpcHandlers
participant SM as SubstateManager
participant DB as SQLite Store
Client->>RPC: "get_substates" (Vec<GetSubstateRequest>)
RPC->>H: dispatch get_substates
alt too many requests (>20)
H-->>Client: JsonRpcError InvalidParams
else valid
H->>SM: get_substates(&[SubstateId])
SM->>DB: create_read_tx() + tx.get_substates(ids)
DB-->>SM: Vec<SubstateResponse>
SM-->>H: HashMap<SubstateId, Substate>
H-->>Client: GetSubstatesResponse { substates }
end
note right of H: Errors → mapped to internal_error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
clients/tari_indexer_client/src/types.rs (2)
96-109: Add minimal docs to clarify batch request semanticsRecommend documenting that this wrapper is used by both get_substates and fetch_substates, and any ordering expectations across request/response.
Apply this diff to add clarifying comments:
#[derive(Debug, Clone, Serialize, Deserialize)] #[cfg_attr( feature = "ts", derive(ts_rs::TS), ts( export, export_to = "../../bindings/src/types/tari-indexer-client/", rename = "IndexerGetSubstatesRequest" ) )] +/// Batch request wrapper for fetching multiple substates in one call. +/// The response entries are expected to align by index with this `requests` vector. pub struct GetSubstatesRequest { pub requests: Vec<GetSubstateRequest>, }
110-122: Clarify 1:1 ordering and NotFound semantics in the batch responseThe Option type is clear, but a brief doc comment will help consumers rely on positional correspondence and "not found" cases.
Apply this diff to add clarifying comments:
#[derive(Debug, Clone, Serialize, Deserialize)] #[cfg_attr( feature = "ts", derive(ts_rs::TS), ts( export, export_to = "../../bindings/src/types/tari-indexer-client/", rename = "IndexerGetSubstatesResponse" ) )] +/// Batch response for multi-substate requests. +/// The i-th entry corresponds to `requests[i]`; `None` indicates not found or unavailable. pub struct GetSubstatesResponse { pub responses: Vec<Option<GetSubstateResponse>>, }clients/tari_indexer_client/src/json_rpc_client.rs (1)
98-104: Alias method fetch_substates mirrors get_substatesConsider documenting that both endpoints are equivalent to reduce API surface confusion. Optionally, route this to call
get_substatesinternally to keep behavior centralized.Apply this minor refactor to delegate to
get_substates:pub async fn fetch_substates( &mut self, req: GetSubstatesRequest, ) -> Result<GetSubstatesResponse, IndexerClientError> { - self.send_request("fetch_substates", req).await + // Alias of `get_substates` on the server; keep behavior centralized here too. + self.get_substates(req).await }applications/tari_indexer/openrpc.json (1)
284-358: Optional: Add a short note that fetch_substates is an alias of get_substatesHelps external integrators understand they are interchangeable.
Would you like me to add a deprecation note to one of them in the OpenRPC description to reduce surface area later?
Also applies to: 360-434
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
applications/tari_indexer/openrpc.json(1 hunks)applications/tari_indexer/src/json_rpc/server.rs(1 hunks)clients/tari_indexer_client/src/json_rpc_client.rs(2 hunks)clients/tari_indexer_client/src/types.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (5)
applications/tari_indexer/src/json_rpc/server.rs (1)
74-76: Batch endpoints wired correctly; aliasing looks goodBoth "get_substates" and "fetch_substates" route to the same handler, which is fine for aliasing/back-compat. No additional concerns here.
clients/tari_indexer_client/src/types.rs (1)
25-29: TS rename for ListSubstatesRequest is consistentThe rename to "IndexerListSubstatesRequest" aligns with the existing TS naming pattern for other requests.
clients/tari_indexer_client/src/json_rpc_client.rs (2)
38-40: New batch types imported correctlyThe new GetSubstatesRequest/Response imports are correct and used below.
91-97: New get_substates client method — looks goodMethod signature and delegation to send_request are consistent with existing style.
applications/tari_indexer/openrpc.json (1)
10-14: Server URL update to http://localhost:18300 — looks correctMatches the indexer server default in this PR.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
applications/tari_indexer/openrpc.json (2)
331-345: Example params shape still double-nested; make "value" the array directlyThe method param "requests" is an array. The example incorrectly nests under a "requests" key, causing params.requests.requests confusion. It should be the array itself.
Apply this diff:
"params": [ { "name": "requests", - "value": { - "requests": [ - { - "address": "e4773b971b876238475c904927435d625d892132e7d7b80c3cc81698fcc4a2dd" - }, - { - "address": "7c45d28ffa167b10d2fa700ce29343de692b97422a65485d0a00e8e7578d692e" - } - ] - } + "value": [ + { + "address": "e4773b971b876238475c904927435d625d892132e7d7b80c3cc81698fcc4a2dd" + }, + { + "address": "7c45d28ffa167b10d2fa700ce29343de692b97422a65485d0a00e8e7578d692e" + } + ] } ],
436-448: Add missing optional fields to GetSubstateRequest schema for batch parityBatch items reference GetSubstateRequest, but this schema only includes "address". In code, version and local_search_only are supported; omitting them prevents accurate docs/codegen for batch requests.
Apply this diff:
"GetSubstateRequest": { "type": "object", "properties": { "address": { "type": "string", "pattern": "^[0-9a-fA-F]+$", "description": "Hex-encoded SubstateId as expected by the JSON API" - } + }, + "version": { + "type": "integer", + "description": "Optional specific substate version to fetch" + }, + "local_search_only": { + "type": "boolean", + "default": false, + "description": "If true, do not query peers when not found locally" + } }, "required": [ "address" ] },
🧹 Nitpick comments (2)
applications/tari_indexer/openrpc.json (2)
487-499: Optional: include a null example in batch result to document missesConsider adding a second entry with null to illustrate not-found results, since responses items allow null.
96-101: Out-of-scope note: empty result schemas for get_identity/get_connectionsThese still use empty schemas and "epoch_stats" as the result name, which is misleading. Not a blocker for this PR, but worth aligning later.
Also applies to: 69-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
applications/tari_indexer/openrpc.json(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
applications/tari_indexer/openrpc.json
[error] 325-325: expected , but instead found ]
Remove ]
(parse)
[error] 372-372: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 373-432: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 433-433: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 434-434: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 434-434: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🔇 Additional comments (2)
applications/tari_indexer/openrpc.json (2)
12-14: Server URL fix LGTMThe switch to http://localhost:18300 aligns with the indexer default port. Good update.
127-150: Param docs for get_substate look goodIncluding optional version and local_search_only matches the server types and improves client ergonomics.
| "component_e4773b971b876238475c904927435d625d892132e7d7b80c3cc81698fcc4a2dd", | ||
| 0 | ||
| ], | ||
| [ | ||
| "component_7c45d28ffa167b10d2fa700ce29343de692b97422a65485d0a00e8e7578d692e", | ||
| 0 | ||
| ], | ||
| [ | ||
| "component_3bca9b603e196b60f261aad0db85e7b55d52c46d687cdbb54194b706423fb1aa", | ||
| 0 | ||
| ] | ||
| ] | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Automated validation script to prevent regressions
Add CI to verify JSON validity and method completeness (name/result present) and ensure example param shapes match the declared types.
Also applies to: 304-372, 373-432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Validate JSON
jq '.' applications/tari_indexer/openrpc.json >/dev/null
# 2) Ensure methods have names and results
missing_meta=$(jq -r '.methods[] | select((.name|not) or (.result|not)) | @json' applications/tari_indexer/openrpc.json || true)
if [[ -n "${missing_meta}" ]]; then
echo "Methods missing name/result:"
echo "${missing_meta}"
exit 1
fi
# 3) Check get_substates/fetch_substates param schemas are arrays
for m in get_substates fetch_substates; do
jq -r --arg m "$m" '
.methods[] | select(.name==$m) | .params[] | select(.name=="requests") | .schema.type
' applications/tari_indexer/openrpc.json | rg -nP '^array$' >/dev/null
done
# 4) Verify examples use array for "requests" values (not nested objects)
bad_examples=$(jq -r '
.methods[] | select(.name=="get_substates" or .name=="fetch_substates") |
.examples[]?.params[]? |
select(.name=="requests") |
select((.value|type) != "array")' applications/tari_indexer/openrpc.json || true)
if [[ -n "${bad_examples}" ]]; then
echo "Found nested/non-array example values for requests."
exit 1
fi
echo "OpenRPC checks passed."Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show context around the JSON parse error
sed -n '320,330p' applications/tari_indexer/openrpc.jsonLength of output: 471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display a broader context to pinpoint the JSON syntax issue
echo "Showing lines 300–350 around the reported parse error:"
sed -n '300,350p' applications/tari_indexer/openrpc.jsonLength of output: 2067
Fix JSON syntax in examples and add CI validation
The openrpc.json file currently contains malformed examples for the get_substates (and similarly fetch_substates) methods: the requests example is wrapped in an extra object, causing an unmatched-bracket parse error and a mismatch with the declared array schema. Please:
• File: applications/tari_indexer/openrpc.json
– Around lines 323–333, update the value for the requests param in the get_substates example to be an array directly, not an object with a requests key.
• File: applications/tari_indexer/openrpc.json
– Around lines 380–390, apply the same fix in the fetch_substates example.
• Add a CI job (e.g., GitHub Actions) that runs a validation script to:
jq-validate the JSON.- Ensure every method has both
nameandresult. - Confirm that the
requestsparam in these methods declares"type":"array". - Verify all
examples[].params[].valueforrequestsare arrays, not nested objects.
Example diff for get_substates (similar changes apply to fetch_substates):
--- a/applications/tari_indexer/openrpc.json
+++ b/applications/tari_indexer/openrpc.json
@@ -323,12 +323,12 @@
"params": [
{
"name": "requests",
- "value": {
- "requests": [
+ "value": [
{
"address": "e4773b97…a2dd"
},
{
"address": "7c45d28…8d692e"
}
- ]
- }
+ ]
}
],
"result": {These fixes will restore valid JSON, match the declared schema, and guard against future regressions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In applications/tari_indexer/openrpc.json around lines 110 to 124 (and similarly
around the referenced sections for get_substates at ~323–333 and fetch_substates
at ~380–390), the examples for the requests param are wrapped in an extra object
instead of being a raw array; update those examples so the
examples[].params[].value for the requests param is an array directly (remove
the outer {"requests": ...} object) to match the declared "type":"array". Then
add a CI job (e.g., GitHub Actions) that runs a validation script which: 1)
jq-validates the JSON file, 2) iterates methods to ensure each has both name and
result, 3) confirms the requests param declares "type":"array", and 4) checks
that examples[].params[].value for requests are arrays (fail the job if any of
these checks fail).
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
applications/tari_indexer/openrpc.json (1)
95-105: Fill the get_addresses summary for claritySmall doc polish to aid users browsing the spec.
- "summary": "", + "summary": "List known substate addresses and their latest version.",applications/tari_indexer/src/storage_sqlite/store_factory.rs (2)
242-247: Timestamp conversion: improve error contextConversion looks correct (epoch seconds → OffsetDateTime → PrimitiveDateTime). Add more context to the error message to aid debugging (include address and raw timestamp).
- let offset_datetime = tari_ootle_storage::time::OffsetDateTime::from_unix_timestamp(s.timestamp as i64) - .map_err(|e| anyhow::anyhow!("Invalid timestamp: {}", e))?; + let offset_datetime = tari_ootle_storage::time::OffsetDateTime::from_unix_timestamp(s.timestamp as i64) + .map_err(|e| anyhow::anyhow!( + "Invalid timestamp (secs) {} for substate {}: {}", + s.timestamp, s.address, e + ))?;
470-477: Copy-paste error in error messages (“get_last_scanned_block_id”)These log/error strings are misleading in list_recent_transactions.
- reason: format!("get_last_scanned_block_id: {}", e), + reason: format!("list_recent_transactions: {}", e),And similarly:
- reason: format!("get_last_scanned_block_id: {}", e), + reason: format!("list_recent_transactions: {}", e),applications/tari_indexer/src/json_rpc/handlers.rs (3)
321-327: Minor: log/error message spacingNit: add a space before the error to improve readability in the message.
- format!("Error asking network for substate:{}", e), + format!("Error asking network for substate: {}", e),
399-405: Minor: message spacing (batch path)Mirror the earlier nit in the batch network error path.
- format!("Error asking network for substate:{}", e), + format!("Error asking network for substate: {}", e),
368-426: Optional: process batch requests concurrentlyEach request is awaited sequentially, which can be slow for larger batches. Consider bounded concurrency (e.g., FuturesUnordered with a cap) to reduce tail latency.
Sketch:
use futures::{stream, StreamExt}; let concurrency = 16usize; let responses = stream::iter(requests) .map(|request| async move { // existing per-request logic… }) .buffer_unordered(concurrency) .collect::<Vec<_>>() .await;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
applications/tari_indexer/Cargo.toml(1 hunks)applications/tari_indexer/openrpc.json(1 hunks)applications/tari_indexer/src/json_rpc/handlers.rs(4 hunks)applications/tari_indexer/src/storage_sqlite/store_factory.rs(3 hunks)clients/tari_indexer_client/src/types.rs(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (3)
bindings/src/types/tari-indexer-client/ListSubstateItem.ts (1)
ListSubstateItem(5-11)bindings/src/types/tari-indexer-client/TransactionEntry.ts (1)
TransactionEntry(5-5)bindings/src/types/tari-indexer-client/IndexerTransactionFinalizedResult.ts (1)
IndexerTransactionFinalizedResult(5-15)
applications/tari_indexer/src/json_rpc/handlers.rs (3)
clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_substates(91-96)applications/tari_validator_node/src/json_rpc/jrpc_errors.rs (1)
internal_error(47-60)bindings/src/types/tari-indexer-client/IndexerTransactionFinalizedResult.ts (1)
IndexerTransactionFinalizedResult(5-15)
clients/tari_indexer_client/src/types.rs (13)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/UtxoTagByte.ts (1)
UtxoTagByte(3-3)bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)bindings/src/types/tari-indexer-client/ListSubstateItem.ts (1)
ListSubstateItem(5-11)bindings/src/types/validator-node-client/TemplateMetadata.ts (1)
TemplateMetadata(4-11)bindings/src/types/tari-indexer-client/IndexerTransactionFinalizedResult.ts (1)
IndexerTransactionFinalizedResult(5-15)bindings/src/types/tari-indexer-client/TransactionEntry.ts (1)
TransactionEntry(5-5)bindings/src/types/tari-indexer-client/GetNonFungiblesRequest.ts (1)
GetNonFungiblesRequest(4-4)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/tari-indexer-client/NonFungibleSubstate.ts (1)
NonFungibleSubstate(5-5)bindings/src/types/tari-indexer-client/GetTemplateDefinitionResponse.ts (1)
GetTemplateDefinitionResponse(4-4)bindings/src/types/Decision.ts (1)
Decision(4-4)bindings/src/types/ExecuteResult.ts (1)
ExecuteResult(4-10)
🔇 Additional comments (8)
applications/tari_indexer/Cargo.toml (1)
65-65: Add time v0.3 with serde: OK — ensure workspace-wide consistencyLooks good. Please double-check that all crates serializing/deserializing time::PrimitiveDateTime or OffsetDateTime have the "time" crate compiled with the serde feature (especially clients/tari_indexer_client) to avoid mismatched wire formats.
clients/tari_indexer_client/src/types.rs (3)
49-51: Explicit JSON format for timestamps is missingPrimitiveDateTime’s serde format isn’t self-evident. To guarantee a stable, human-readable wire format, explicitly serialize as RFC3339.
[ suggest_essential_refactor ]Apply this diff:
pub struct ListSubstateItem { pub substate_id: SubstateId, pub module_name: Option<String>, pub version: u32, pub template_address: Option<TemplateAddress>, - #[cfg_attr(feature = "ts", ts(type = "string"))] - pub timestamp: PrimitiveDateTime, + #[cfg_attr(feature = "ts", ts(type = "string"))] + #[serde(with = "time::serde::rfc3339")] + pub timestamp: PrimitiveDateTime, }Note: This requires the client crate to depend on
timewithfeatures = ["serde"]. If that’s not already true, add it to clients/tari_indexer_client/Cargo.toml.
191-193: Align TransactionEntry.timestamp to RFC3339Same rationale as above; make the JSON format explicit.
[ suggest_essential_refactor ]pub struct TransactionEntry { pub transaction_id: TransactionId, pub status: IndexerTransactionFinalizedResult, pub fee: u64, - pub timestamp: PrimitiveDateTime, + #[cfg_attr(feature = "ts", ts(type = "string"))] + #[serde(with = "time::serde::rfc3339")] + pub timestamp: PrimitiveDateTime, }
373-376: Finalize time: use RFC3339 for finalized_time (and keep Duration as {secs,nanos})Ensure consistency across all date-time fields.
[ suggest_essential_refactor ]Finalized { final_decision: Decision, execution_result: Option<Box<ExecuteResult>>, #[cfg_attr(feature = "ts", ts(type = "{secs: number, nanos: number}"))] execution_time: Duration, - #[cfg_attr(feature = "ts", ts(type = "string"))] - finalized_time: PrimitiveDateTime, + #[cfg_attr(feature = "ts", ts(type = "string"))] + #[serde(with = "time::serde::rfc3339")] + finalized_time: PrimitiveDateTime, abort_details: Option<String>, },applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
479-484: TransactionEntry fields populated with defaultsStatus Pending and fee 0 are fine as placeholders. Confirm front-end expectations: PR summary mentions “total fees” and “retain created_at”. You’ve renamed created_at to timestamp and dropped the transaction body. Ensure the UI and TS bindings are updated accordingly.
applications/tari_indexer/src/json_rpc/handlers.rs (3)
209-210: add_peer now returns a concrete response — goodReturning AddPeerResponse { success: true } improves client ergonomics.
147-161: get_identity fields diverge from OpenRPCHandler returns peer_id and public_addresses (vector). OpenRPC still documents node_id/public_address (single). Update OpenRPC to match this handler (recommended), or change the handler response to match OpenRPC. See the OpenRPC review comment with a proposed schema.
695-696: Pagination param removed — confirm client expectationsYou’ve hard-coded last_transaction_id = None and removed last_id from the request type. Confirm that consumers (Indexer Web UI) don’t rely on pagination via last_id anymore, or add a replacement.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ghpbot-tari-project
removed
the
P-conflicts
ghpbot-tari-project
added
the
P-conflicts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_indexer/src/json_rpc/handlers.rs (1)
40-71: Missing import for GetSubstatesResponse (and GetSubstatesRequest) — compile errorThe handler returns GetSubstatesResponse at Line 425 but this type is not imported. Also, if you switch parsing to GetSubstatesRequest (see next comment), import that too.
Apply this diff to fix the missing imports:
use tari_indexer_client::types::{ self, AddPeerRequest, AddPeerResponse, ConnectionDirection, GetCommsStatsResponse, GetConnectionsResponse, GetEpochManagerStatsResponse, GetIdentityResponse, GetNonFungiblesRequest, GetNonFungiblesResponse, GetSubstateRequest, GetSubstateResponse, + GetSubstatesRequest, + GetSubstatesResponse, GetTemplateDefinitionRequest, GetTemplateDefinitionResponse, GetTransactionResultRequest, GetTransactionResultResponse, IndexerReadyResponse, IndexerTransactionFinalizedResult, InspectSubstateRequest,
♻️ Duplicate comments (3)
applications/tari_indexer/openrpc.json (3)
39-44: Example does not match server Connection shape; update fieldsExample uses boolean direction and integer age; server returns direction as "Inbound"/"Outbound" and age as Duration (secs/nanos), plus connection_id, optional ping_latency and user_agent. Align the example.
Apply this diff:
- "address": "/onion3/zqydnqgtapphitwdxr4sfg5kuvv7kbs5ye54yjy2mybi7f7qxd5jeiyd:18141", - "age": 9946, - "direction": false, - "peer_id": "12D3KooWJn8z...tVx", - "public_key": "b4a0b93ccd3a50ab8f7fb766cc9fea59e574283d69a53fe60d6aa842f42ecd38" + "connection_id": "c4101a6e-6c2e-4cfa-9e49-2c85b934a23a", + "peer_id": "12D3KooWJn8z...tVx", + "address": "/onion3/zqydnqgtapphitwdxr4sfg5kuvv7kbs5ye54yjy2mybi7f7qxd5jeiyd:18141", + "direction": "Outbound", + "age": { "secs": 9946, "nanos": 0 }, + "ping_latency": null, + "user_agent": "tari-indexer/0.1.0"
473-507: Connection schema mismatches server/types; align fields and typesReplace the legacy node_id/public_key, boolean direction, integer age with the actual server shape.
Apply this diff:
"Connection": { "type": "object", "properties": { - "address": { "type": "string", "description": "Multiaddress (onion3/TCP)" }, - "age": { "type": "integer", "minimum": 0, "description": "Connection age in seconds" }, - "direction": { "type": "boolean", "description": "true=inbound, false=outbound" }, - "node_id": { - "type": "string", - "pattern": "^[0-9a-fA-F]+$", - "description": "Hex-encoded peer NodeId" - }, - "public_key": { - "type": "string", - "pattern": "^[0-9a-fA-F]+$", - "description": "Hex-encoded public key" - } + "connection_id": { "type": "string" }, + "peer_id": { "type": "string", "description": "Libp2p PeerId string" }, + "address": { "type": "string", "description": "Multiaddr" }, + "direction": { "type": "string", "enum": ["Inbound", "Outbound"] }, + "age": { + "type": "object", + "properties": { "secs": { "type": "integer" }, "nanos": { "type": "integer" } }, + "required": ["secs","nanos"] + }, + "ping_latency": { + "type": "object", + "nullable": true, + "properties": { "secs": { "type": "integer" }, "nanos": { "type": "integer" } }, + "required": ["secs","nanos"] + }, + "user_agent": { "type": "string", "nullable": true } }, "required": [ - "address", - "age", - "direction", - "node_id", - "public_key" + "connection_id", + "peer_id", + "address", + "direction", + "age" ] }
508-529: GetIdentityResponse schema mismatches implementation; use peer_id/public_addressesServer returns { peer_id, public_key, public_addresses[] }. Update the component schema and required set.
Apply this diff:
"GetIdentityResponse": { "type": "object", "properties": { - "node_id": { - "type": "string", - "pattern": "^[0-9a-fA-F]+$", - "description": "Hex-encoded NodeId" - }, - "public_address": { "type": "string" }, - "public_key": { - "type": "string", - "pattern": "^[0-9a-fA-F]+$" - } + "peer_id": { "type": "string", "description": "Libp2p PeerId string" }, + "public_addresses": { "type": "array", "items": { "type": "string" } }, + "public_key": { "type": "string" } }, "required": [ - "node_id", - "public_address", - "public_key" + "peer_id", + "public_addresses", + "public_key" ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
applications/tari_indexer/openrpc.json(1 hunks)applications/tari_indexer/src/json_rpc/handlers.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
applications/tari_indexer/src/json_rpc/handlers.rs (4)
clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_substates(91-96)applications/tari_validator_node/src/json_rpc/jrpc_errors.rs (1)
internal_error(47-60)bindings/src/types/SubstateRequirement.ts (1)
SubstateRequirement(3-3)bindings/src/types/tari-indexer-client/IndexerTransactionFinalizedResult.ts (1)
IndexerTransactionFinalizedResult(5-15)
🪛 Biome (2.1.2)
applications/tari_indexer/openrpc.json
[error] 71-71: expected , but instead found "examples"
Remove "examples"
(parse)
🔇 Additional comments (3)
applications/tari_indexer/src/json_rpc/handlers.rs (2)
425-426: Good: returning the documented container typeUsing GetSubstatesResponse { responses } aligns the server with OpenRPC and client types. Once imports are fixed, this will compile and interop cleanly.
693-699: API behavior change in list_recent_transactions — confirm pagination semanticsSwitching the first arg to None changes the starting point. Verify UI expectations (recency order, last_id handling) and adjust the OpenRPC method docs/examples if needed.
Would you like a quick script to fetch before/after samples and compare ordering?
applications/tari_indexer/openrpc.json (1)
321-387: Sanity check: batch methods parameter/result schemas look good
- requests param is an array of GetSubstateRequest (no double-nesting).
- result references GetSubstatesResponse with item anyOf GetSubstateResponse|null.
Assuming the server returns the container (it does), this aligns well.
Also applies to: 388-455
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ghpbot-tari-project
removed
the
P-conflicts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ghpbot-tari-project
added
the
P-conflicts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
applications/tari_indexer/src/json_rpc/handlers.rs (1)
666-676: Switch tostatusfield for GetTransactionResultResponse looks correct; ensure bindings are regeneratedServer now emits
{ status: ... }. Make sure TS/other SDK bindings and UI callers expect.status(not.result). There’s already a note in prior reviews about TS bindings.Run this quick check after regenerating bindings:
#!/usr/bin/env bash set -euo pipefail rg -nP -C2 '\bGetTransactionResultResponse\b' bindings/ clients/ | cat rg -nP -C2 '\b\.result\b' bindings/ clients/ web_ui/ || true rg -nP -C2 '\b\.status\b' bindings/ clients/ web_ui/ | cat
🧹 Nitpick comments (4)
applications/tari_indexer/src/json_rpc/handlers.rs (4)
401-404: Add context to warn log for degraded network errorsIncluding the address/version greatly improves debuggability of partial failures.
Apply this diff:
- Err(e) => { - warn!(target: LOG_TARGET, "Error asking network for substate: {}", e); + Err(e) => { + warn!( + target: LOG_TARGET, + "Error asking network for substate {} (version {:?}): {}", + request.address, + request.version, + e + ); responses.push(None); },
376-383: Consider not aborting the whole batch on local store errorsRight now, any local DB error via
map_err(Self::internal_error(...))?aborts the entire batch. For a batch endpoint, returningNonefor just the failing item is often preferable, mirroring how you handled network errors.Apply this diff to degrade local errors to
Noneand continue:- let maybe_substate = self - .substate_manager - .get_substate(&request.address, request.version) - .await - .map_err(|e| { - warn!(target: LOG_TARGET, "Error getting substate: {}", e); - Self::internal_error(answer_id, format!("Error getting substate: {}", e)) - })?; + let maybe_substate = match self + .substate_manager + .get_substate(&request.address, request.version) + .await + { + Ok(v) => v, + Err(e) => { + warn!( + target: LOG_TARGET, + "Local store error getting substate {} (version {:?}): {}", + request.address, request.version, e + ); + responses.push(None); + continue; + } + };If keeping the current behavior is intentional (e.g., DB corruption signal), consider documenting it in the OpenRPC description.
Would you like me to update the OpenRPC schema description to clarify the per-item vs. whole-batch error behavior?
614-616: Minor typo in comment“ad there is no DB type for that” → “as there is no DB type for that”.
Apply this diff:
- // TemplateExecutable::DownloadableWasm is never returned ad there is no DB type for that + // TemplateExecutable::DownloadableWasm is never returned as there is no DB type for that
368-431: Optional: parallelize network fetches with bounded concurrency while preserving orderFor large batches, sequential network calls can be slow. Consider using a small concurrency limit (e.g., 16–32) with
futures::stream::iter(...).buffer_unordered(N), carrying indices to restore order. This can reduce p95 latency substantially while keeping memory bounded.If you want, I can draft a concrete refactor using
futureswith order preservation and minimal churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
applications/tari_indexer/src/json_rpc/handlers.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
applications/tari_indexer/src/json_rpc/handlers.rs (4)
clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_substates(91-96)applications/tari_validator_node/src/json_rpc/jrpc_errors.rs (1)
internal_error(47-60)bindings/src/types/SubstateRequirement.ts (1)
SubstateRequirement(3-3)bindings/src/types/tari-indexer-client/IndexerTransactionFinalizedResult.ts (1)
IndexerTransactionFinalizedResult(5-15)
🔇 Additional comments (1)
applications/tari_indexer/src/json_rpc/handlers.rs (1)
699-701: I’ve requested the handler context, trait/impl signatures, and the request struct to ensure the server is passinglast_transaction_idcorrectly and that all signatures align. Let me know once the outputs are in, then I’ll confirm and update the review.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ghpbot-tari-project
added
P-conflicts
|
@saa938 Thanks for the PR, mind if I finish it up? |
* development: (26 commits) feat(wallet): add stealth_utxos.list rpc call (#1576) feat(template_lib): adds engine schnorr signature verification (#1574) feat(wallet)!: add bech32 address with view-only key (#1573) feat(walletui): wallet ux improvements (#1572) fix(wallet)!: private derived tag and optimised* sync protocol (#1571) feat(walletui): send flow ux improvements (#1570) doc: update openrpc.json get_connections method (#1567) chore(deps): bump actions/setup-node from 4 to 5 (#1565) refactor(wallet): move indexer backend into own crate (#1566) chore(walletui): update dependencies (#1564) test: fix several cucumbers (#1563) chore!: upgrade to minotari 5.0.0 (#1560) chore(deps): bump actions/checkout from 4 to 5 (#1545) chore(deps): bump tracing-subscriber from 0.3.19 to 0.3.20 (#1562) feat(walletui): add transaction timechip and update transaction details (#1561) fix(engine): limit number of logs and events permitted (#1559) fix: revert "use npm instead of pnpm (#1553)" (#1558) feat!: migrate XTR to stealth resource (#1556) docs: updated prerequisites (#1554) fix: use npm instead of pnpm (#1553) ...
ghpbot-tari-project
removed
the
P-conflicts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx (2)
129-132: React list keys should be stable and unique (avoid using index).Using the index as a key can cause incorrect reuse/reordering artifacts. Prefer the NFT’s stable address.
- {nfts.map((item, i) => + {nfts.map((item) => item.img ? ( - <ImageListItem key={i}> + <ImageListItem key={item.address}> ... </ImageListItem> ) : ( - <ImageListItem key={i}> + <ImageListItem key={item.address}> ... </ImageListItem> ), )}Also applies to: 149-160
73-103: Batch the state update: move setNfts out of the loop.Calling
setNftsper item triggers N re-renders. Build the array, then update once.- let nfts: NftData[] = []; - resp.non_fungibles.forEach((nft: NonFungibleSubstate) => { + const next: NftData[] = []; + resp.non_fungibles.forEach((nft: NonFungibleSubstate) => { ... - if (nftData) { + if (nftData) { ... - nfts.push({ + next.push({ img: image_url, title: name, address, version: nft.version, }); } - - setNfts(nfts); }); + setNfts(next);applications/tari_walletd/src/handlers/substates.rs (1)
64-69: Guard against unbounded list limits.
If not enforced downstream, consider clampingreq.limitto a sane maximum to prevent large payloads and memory spikes.Example diff (adjust types as needed):
+const MAX_SUBSTATE_LIST_LIMIT: u64 = 1000; @@ - let substates = sdk.substate_api().list_substates( + let limit = req.limit.map(|n| n.min(MAX_SUBSTATE_LIST_LIMIT)); + let substates = sdk.substate_api().list_substates( req.filter_by_type, req.filter_by_template.as_ref(), - req.limit, + limit, req.offset, )?;applications/tari_indexer/src/storage_sqlite/reader.rs (1)
146-160: Consider optimizing the batch query for better performance.The current implementation has a few areas that could be improved:
- Converting SubstateId to strings creates unnecessary allocations
- Missing ordering which could lead to inconsistent results
- No version filtering like the single
get_substatemethodConsider these optimizations:
fn get_substates(&mut self, ids: &[SubstateId]) -> Result<Vec<SubstateResponse>, StorageError> { use crate::storage_sqlite::schema::substates; let str_ids = ids.iter().map(|id| id.to_string()); let rows = substates::table .select(substates::all_columns) .filter(substates::address.eq_any(str_ids)) + .order_by(substates::address.asc()) + .then_order_by(substates::version.desc()) .get_results::<SubstateRecord>(self.connection()) .map_err(|e| StorageError::QueryError { - reason: format!("get_substate: {}", e), + reason: format!("get_substates: {}", e), })?; rows.into_iter().map(TryInto::try_into).collect() }Additionally, consider if the batch API should support version filtering like the single substate API does.
applications/tari_indexer/src/json_rpc/handlers.rs (1)
397-399: Consider more resilient error handling for batch operations.Currently, if getting substates fails, the entire batch request fails. Consider returning partial results with errors for specific substates, similar to how GraphQL handles batch operations.
Consider a more resilient approach:
- let substates = self.substate_manager.get_substates(requests.as_slice()).map_err(|e| { - warn!(target: LOG_TARGET, "Error getting substate: {}", e); - Self::internal_error(answer_id, format!("Error getting substate: {}", e)) - })?; + let substates = match self.substate_manager.get_substates(requests.as_slice()) { + Ok(substates) => substates, + Err(e) => { + warn!(target: LOG_TARGET, "Error getting substates, returning empty map: {}", e); + // Consider if returning an empty map or partial results is more appropriate + HashMap::new() + } + };Or alternatively, modify the response type to include per-substate errors if the API design allows for it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
applications/tari_indexer/Cargo.toml(1 hunks)applications/tari_indexer/src/json_rpc/handlers.rs(4 hunks)applications/tari_indexer/src/json_rpc/server.rs(1 hunks)applications/tari_indexer/src/storage_sqlite/models/substate.rs(2 hunks)applications/tari_indexer/src/storage_sqlite/reader.rs(2 hunks)applications/tari_indexer/src/storage_sqlite/store_factory.rs(2 hunks)applications/tari_indexer/src/substate_manager.rs(2 hunks)applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx(4 hunks)applications/tari_walletd/src/handlers/substates.rs(3 hunks)applications/tari_walletd/src/jrpc_server.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/tari_walletd/src/jrpc_server.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- applications/tari_indexer/src/storage_sqlite/store_factory.rs
- applications/tari_indexer/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (5)
applications/tari_indexer/src/storage_sqlite/models/substate.rs (1)
applications/tari_indexer/src/storage_sqlite/serialization.rs (1)
deserialize_json(36-42)
applications/tari_indexer/src/storage_sqlite/reader.rs (3)
applications/tari_indexer/src/json_rpc/handlers.rs (1)
get_substates(382-403)applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
get_substates(141-141)applications/tari_indexer/src/substate_manager.rs (1)
get_substates(163-170)
applications/tari_walletd/src/handlers/substates.rs (2)
bindings/src/types/wallet-daemon-client/WalletSubstateInfo.ts (1)
WalletSubstateInfo(4-10)crates/engine_types/src/substate.rs (2)
new(63-68)new(833-839)
applications/tari_indexer/src/substate_manager.rs (4)
bindings/src/types/Substate.ts (1)
Substate(4-4)applications/tari_indexer/src/storage_sqlite/reader.rs (5)
get_substates(146-160)substates(98-113)substates(151-154)substates(184-188)new(54-56)applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
get_substates(141-141)crates/engine_types/src/substate.rs (2)
new(63-68)new(833-839)
applications/tari_indexer/src/json_rpc/handlers.rs (7)
bindings/src/types/tari-indexer-client/GetSubstatesRequest.ts (1)
GetSubstatesRequest(4-4)bindings/src/types/tari-indexer-client/GetSubstatesResponse.ts (1)
GetSubstatesResponse(5-5)applications/tari_indexer/src/storage_sqlite/reader.rs (4)
get_substates(146-160)substates(98-113)substates(151-154)substates(184-188)applications/tari_indexer/src/substate_manager.rs (1)
get_substates(163-170)crates/wallet/sdk/src/network.rs (1)
get_substates(36-39)crates/wallet/sdk_services/src/indexer_jrpc_impl.rs (1)
get_substates(105-119)clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_substates(95-100)
🔇 Additional comments (9)
applications/tari_walletd/src/handlers/substates.rs (3)
11-16: Imports updated to new WalletSubstateInfo — LGTM.
Type alignment with daemon client looks correct.
71-79: Parent_id/source field parity verified.
s.parent_address and WalletSubstateInfo.parent_id are both Option (crates/wallet/sdk/src/models/substate.rs ↔ clients/wallet_daemon_client/src/types.rs); mapping is type-safe — no change required.
46-54: Confirm query_substate version semantics (exact vs. since)Mapping to WalletSubstateInfo and Substate::new looks correct; nice split of local vs remote. WalletNetworkInterface::query_substate (crates/wallet/sdk/src/network.rs) and IndexerJsonRpcNetworkInterface::query_substate (crates/wallet/sdk_services/src/indexer_jrpc_impl.rs) forward the Option version into the indexer's GetSubstateRequest (type lives in external tari_indexer_client). State whether the version argument means "exact at version" (return only if the substate is at that version) or "since version" (return latest if newer) to avoid stale reads.
applications/tari_indexer/src/storage_sqlite/models/substate.rs (2)
51-65: LGTM! Improved error handling with structured errors.The conversion from
anyhow::ErrortoStorageErrorprovides better structured error handling with detailed context about decoding failures.
26-26: Resolved — time crate's serde-human-readable feature is enabled.Verified: crates/storage/Cargo.toml sets time = { workspace = true, features = ["serde", "serde-human-readable"] } and crates/storage/src/lib.rs re-exports time, so tari_ootle_storage::time::PrimitiveDateTime has serde support.
applications/tari_indexer/src/json_rpc/server.rs (1)
78-78: LGTM! New batch API endpoint properly integrated.The new
get_substatesendpoint is correctly wired to delegate to the handler implementation.applications/tari_indexer/src/substate_manager.rs (1)
163-170: LGTM! Clean implementation of batch substate retrieval.The method properly retrieves multiple substates and returns them as a HashMap for efficient lookup.
applications/tari_indexer/src/json_rpc/handlers.rs (2)
819-819: Confirm passing None instead of req.last_id is intentionalhandlers.rs calls .list_recent_transactions(None, limit) which ignores the request's last_id pagination field; the request type and TransactionManager signature accept last_id.
Locations: applications/tari_indexer/src/json_rpc/handlers.rs:819 (call), clients/tari_indexer_client/src/types.rs (ListRecentTransactionsRequest has last_id), applications/tari_indexer/src/transaction_manager/mod.rs:109-116 (fn list_recent_transactions(last_id: Option, ...)).
If unintended, revert to .list_recent_transactions(req.last_id, limit as usize). If intentional, add a comment and update client/docs to avoid breaking caller expectations.
382-403: Types align; multi-get doesn't support per-item versions — confirm intended behavior
TS/Rust type check: Rust defines GetSubstatesResponse { substates: HashMap<SubstateId, Substate> } and the generated TS is GetSubstatesResponse = { substates: { [key in SubstateId]?: Substate } } — no type mismatch. (clients/tari_indexer_client/src/types.rs, bindings/src/types/tari-indexer-client/GetSubstatesResponse.ts)
API/behavior note: get_substates accepts only a list of SubstateId (GetSubstatesRequest.requests) and enforces MAX_REQUESTS = 20; the single get (GetSubstateRequest) supports version: Option. Decide whether to:
- add per-item version support to the multi-get request, or
- document/accept the limitation (handler intentionally returns latest), or
- provide a separate endpoint for versioned bulk fetches.
Also note the deserialization bound is up to 50 in the types but the handler caps at 20 (clients/tari_indexer_client/src/types.rs, applications/tari_indexer/src/json_rpc/handlers.rs).
| const nftId = nft.address.id; | ||
| const key = Object.keys(nftId)[0]; | ||
| const address = `${key}_${nftId[key as keyof typeof nftId]}`; | ||
| nfts.push({ |
There was a problem hiding this comment.
Use canonical address formatter; avoid Object.keys-based derivation
Relying on the “first key” of nft.address.id is brittle and can yield non-canonical or [object Object] values. Use the bindings’ formatter for a stable, canonical string.
- const { image_url, name } = nftData;
- const nftId = nft.address.id;
- const key = Object.keys(nftId)[0];
- const address = `${key}_${nftId[key as keyof typeof nftId]}`;
+ const { image_url, name } = nftData;
+ const address = substateIdToString(nft.address);📝 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.
| const nftId = nft.address.id; | |
| const key = Object.keys(nftId)[0]; | |
| const address = `${key}_${nftId[key as keyof typeof nftId]}`; | |
| nfts.push({ | |
| const { image_url, name } = nftData; | |
| const address = substateIdToString(nft.address); | |
| nfts.push({ |
🤖 Prompt for AI Agents
In applications/tari_indexer/web_ui/src/routes/VN/Components/Resources.tsx
around lines 90 to 93, replace the brittle Object.keys-based derivation of
address with the bindings' canonical address formatter: remove the
Object.keys/first-key logic and call the provided formatter on the address
object (for example use the bindings' format function or the address object's
toString/canonical method) to produce a stable canonical string, then use that
string for the address variable before pushing into nfts.
* development: feat: batch fetch substates api (tari-project#1547) feat(wallet): add stealth_utxos.list rpc call (tari-project#1576)
3 participants
Description
This PR introduces a new batch API endpoint for fetching substates in the indexer and enhances the "Recent Transactions" list in the Indexer Web UI.
Motivation and Context
The new get_substates JSON-RPC API allows clients, such as wallets, to efficiently retrieve multiple substates in a single batched request, reducing the need for numerous individual calls and improving network efficiency.
The improvements to the Indexer Web UI's "Recent Transactions" list aim to provide a more comprehensive and user-friendly overview of transaction details, including their status and total fees, aligning with the user experience of the Ootle wallet. The created_at timestamp is also retained for better transaction tracking.
ref #1546
closes #1538
How Has This Been Tested?
The changes were validated by compiling the Rust tari_indexer and tari_indexer_client crates and building the tari_indexer/web_ui to ensure correct rendering of UI components and integration of new data.
What process can a PR reviewer use to test or verify this change?
Verify Batch Substate API (get_substates) and Verify Indexer Web UI Transaction List Improvements
Breaking Changes
[x]
Summary by CodeRabbit
New Features
Improvements
UI