Add zkApp command range query#162
Conversation
Hebilicious
force-pushed
the
branch/ext-21-zkapp-command-range-query
branch
from
23d7ca3 to
5a4b75a
Compare
|
@Hebilicious thanks for contribution! I just briefly looked at it.. will review this PR fully shortly. Looks like our CI has some issueswith running pr on forks |
| import { BlockStatusFilter } from '../../../blockchain/types.js'; | ||
| import type { ZkappCommandDatabaseRow } from './types.js'; | ||
|
|
||
| export function getZkappCommandsQuery( |
There was a problem hiding this comment.
This 250-line query isn't executed by any test, from what i see. The unit tests only cover the pure rowsToZkappCommands and the guard functions, and the integration fixture has only failed zkApp commands (tests/integration/integration.test.ts:10), so with the status <> 'failed' filter it returns empty here.
Can you add test ?
| ON b.id = bzkc.block_id | ||
| AND bzkc.status <> 'failed' | ||
| JOIN zkapp_commands zkc ON bzkc.zkapp_command_id = zkc.id | ||
| JOIN zkapp_fee_payer_body fpb ON zkc.zkapp_fee_payer_body_id = fpb.id |
There was a problem hiding this comment.
zkapp_fee_payer_body is the one table this query JOINs that isn't in USED_TABLES (src/db/sql/events-actions/queries.ts:427). Please add it to USED_TABLES so a missing table is caught at boot rather than first query.
| WHERE | ||
| b.height >= ${from} | ||
| AND b.height < ${to} | ||
| ${ |
There was a problem hiding this comment.
Hm.. This filters purely on b.height and b.chain_status, unlike events/actions which run the recursive pending_chain walk to resolve the true chain. For CANONICAL that's fine, but for the default ALL and for PENDING, competing/orphaned blocks at the same height all come through. Either use canonical resolution logic or document this on the schema field.
| FROM block_range b | ||
| JOIN blocks_zkapp_commands bzkc | ||
| ON b.id = bzkc.block_id | ||
| AND bzkc.status <> 'failed' |
There was a problem hiding this comment.
Here you are filtering out failed commands, so the status is always 'applied' and failureReason always null. The two output fields are effectively constant and looks like there is no point returning them . Given the PR scope is "successful" commands, simplest is to drop both fields.
| tracingState: new TracingState(graphQLSpan), | ||
| }); | ||
| }, | ||
| zkappCommands: async (_, { input }, context) => { |
There was a problem hiding this comment.
events/block-details are gated behind ENABLE_BLOCK_TRANSACTION_DETAILS; this heavier query is exposed ungated. is it intentional?
| """ | ||
| Mina block height to filter zkApp commands to, exclusive | ||
| """ | ||
| to: Int! |
There was a problem hiding this comment.
NIT: from before to reads more naturally.
|
@Hebilicious overall looking good! Just couple comments |
|
@dkijania thanks for the review, I will make the changes you suggest and we will test this on our archive internally. |
Hebilicious
force-pushed
the
branch/ext-21-zkapp-command-range-query
branch
from
5a4b75a to
3d391d9
Compare
2 participants
Closes #161.
Adds
Query.zkappCommandsfor fetching successful zkApp commands over an explicit block range, including block info, fee payer data, and ordered account updates with actions, events, app state, and preconditions.Guardrails:
fromandto; there is no implicit latest-range scanZKAPP_COMMAND_RANGE_SIZEfor this heavier query, defaulting to1000ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT, defaulting to5000accountPublicKey/tokenIdfiltered countsAlso adds service tests for the range and account-update guardrails.