Skip to content

Adjusting to new graphql API#418

Open
dewabisma wants to merge 2 commits intomainfrom
beast/new_graphql_transactions_history
Open

Adjusting to new graphql API#418
dewabisma wants to merge 2 commits intomainfrom
beast/new_graphql_transactions_history

Conversation

@dewabisma
Copy link
Collaborator

@dewabisma dewabisma commented Mar 10, 2026

Summary

Since we changed the reversible transfer in new subsquid, I make adjustment to our quantus app graphql query and also the implementations. I changed get by hash for checking executed transfer because it's not straightforward now we already have executed event and we better use txId for it.

I also deleted unused method and queries since it's already combined in account events now.

We shouldn't merge this PR unless we have deployed the new subsquid. Which I haven't done yet.

@dewabisma dewabisma requested a review from n13 March 10, 2026 11:00
@n13
Copy link
Collaborator

n13 commented Mar 10, 2026

Now I have the full picture. Here's my review of PR #418 - "Adjusting to new graphql API" by dewabisma.


Overview

This PR migrates from the old subsquid API (separate events queries per transaction type with client-side merging) to a new unified accountEvents API. It's a solid architectural improvement: 3 parallel GraphQL queries replaced by 1, simpler pagination with a single offset instead of 4, and proper server-side totalCount for "hasMore" logic. Net result is -49 lines.


What's Good

  • Single query instead of three -- eliminates the complex _fetchSingleType / _fetchOtherTransfers / Future.wait machinery. Much simpler to reason about and debug.
  • Pagination is cleaner -- one offset instead of tracking transfersOffset, reversibleOffset, rewardsOffset, scheduledOffset separately. The totalCount from accountEventsConnection is a proper way to determine hasMore.
  • txId over extrinsicHash for monitoring -- using txId as the identifier for reversible transfers is more semantically correct and aligns with the new data model.
  • Removes dead code -- _checkIfTransferWasExecuted, _fetchSingleType, _fetchOtherTransfers all gone. Good cleanup.

Issues

1. DRY violation in factory constructors

fromCancelledJson and fromExecutedJson are nearly identical -- only the status enum value differs. fromScheduledJson and fromNotificationJson also share the same block-parsing boilerplate. Consider something like:

factory ReversibleTransferEvent.fromExecutedJson(Map<String, dynamic> json) =>
    ReversibleTransferEvent._fromNestedJson(json, ReversibleTransferStatus.EXECUTED);

factory ReversibleTransferEvent.fromCancelledJson(Map<String, dynamic> json) =>
    ReversibleTransferEvent._fromNestedJson(json, ReversibleTransferStatus.CANCELLED);

factory ReversibleTransferEvent._fromNestedJson(Map<String, dynamic> json, ReversibleTransferStatus status) {
  final block = json['block'] as Map<String, dynamic>;
  final scheduledTransfer = json['scheduledTransfer'] as Map<String, dynamic>;
  return ReversibleTransferEvent(
    id: json['id'] as String,
    from: scheduledTransfer['from']?['id'] as String? ?? '',
    to: scheduledTransfer['to']?['id'] as String? ?? '',
    amount: BigInt.parse(scheduledTransfer['amount'] as String),
    timestamp: DateTime.parse(json['timestamp'] as String),
    txId: json['txId'] as String,
    status: status,
    scheduledAt: DateTime.parse(scheduledTransfer['scheduledAt'] as String),
    extrinsicHash: json['extrinsicHash'] as String?,
    blockNumber: block['height'] as int,
    blockHash: block['hash'] as String? ?? '',
  );
}

2. Stale error message

In fetchExecutedTransactionByTxId, the catch block still says:

print('Error fetching transactions by hash: $e');

Should be 'Error fetching executed transaction by txId: $e'.

3. extrinsicHash ghost field in fromExecutedJson

The executedReversibleTransfer fragment in _accountEventsQuery does not fetch extrinsicHash, but fromExecutedJson still reads json['extrinsicHash']. It'll silently be null, which is fine at runtime, but it's misleading. Either add extrinsicHash to the query or remove the read (same for _executedTransactionByTxId query).

4. Deduplication order assumption

In fetchAllTransactionTypes, the reversible transfer deduplication uses containsKey -- first occurrence wins. The if-else chain checks cancelled -> executed -> scheduled. Since events are ordered timestamp_DESC, a newer cancelled/executed event will appear before the original scheduled event, so the latest status wins. This is correct. However, if an event somehow has both cancelledReversibleTransfer and executedReversibleTransfer populated on the same event object, the else if chain would silently skip the executed one. Probably can't happen, but worth a quick sanity check against the new subsquid schema.

5. Minor: trailing whitespace

There's a trailing whitespace on the blank line after:

final transaction = ReversibleTransferEvent.fromExecutedJson(events.first);
      
return transaction;

6. Author's own note

The PR description says: "We shouldn't merge this PR unless we have deployed the new subsquid." -- so this is blocked on infra.


Verdict

Good PR overall -- simplifies the architecture and removes a lot of complexity. The main actionable items are the DRY cleanup on the factory constructors (#1), fixing the stale error message (#2), and aligning the extrinsicHash field between query and parser (#3). Everything else is minor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants