Skip to content

Conversation

@joroshiba
Copy link
Member

Summary

Brief summary of the changes made, ie "what?"

Background

Brief background on why these changes were made, ie "why?"

Changes

  • List changes which were made.

Testing

How are these changes tested?

Changelogs

Ensure all relevant changelog files are updated as necessary. See
keepachangelog for change
categories. Replace this text with e.g. "Changelogs updated." or "No updates
required." to acknowledge changelogs have been considered.

Metrics

  • List out metrics added by PR, delete section if none.

Breaking Changelist

  • Bulleted list of breaking changes, any notes on migration. Delete section if none.

Related Issues

Link any issues that are related, prefer full GitHub links.

closes

@joroshiba joroshiba requested review from a team as code owners November 11, 2025 06:19
@joroshiba joroshiba requested a review from Fraser999 November 11, 2025 06:19
@github-actions github-actions bot added proto pertaining to the Astria Protobuf spec sequencer-relayer pertaining to the astria-sequencer-relayer crate labels Nov 11, 2025
@joroshiba joroshiba added the docker-build used to trigger docker builds on PRs label Nov 11, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 11, 2025

Greptile Overview

Greptile Summary

Switched from Celestia's broken GetTx API to the working TxStatus API for querying transaction status. This change adapts to Celestia's API changes by:

  • Added proto definitions for the celestia.core.v1.tx.Tx service with TxStatus RPC method
  • Generated Rust bindings (TxStatusRequest, TxStatusResponse, TxClient) from the new proto definitions
  • Replaced tx_client.get_tx() calls with blob_tx_client.tx_status() in the confirmation flow
  • Updated response parsing to handle the new TxStatusResponse structure (uses status field instead of checking height alone, and execution_code instead of code)

Issues Found:

  • Dead code: The block_height_from_response() function (lines 559-601) is no longer used after the API switch

Confidence Score: 4/5

  • This PR is safe to merge with minor cleanup recommended
  • The changes correctly implement the API switch from GetTx to TxStatus. The logic properly handles transaction states (pending, committed, failed) with appropriate error handling. Generated code is from vendored proto definitions. Only issue is leftover dead code that should be removed for code cleanliness.
  • Pay attention to crates/astria-sequencer-relayer/src/relayer/celestia_client/mod.rs - contains unused dead code that should be removed

Important Files Changed

File Analysis

Filename Score Overview
proto/vendored/celestia_app/celestia/core/v1/tx/tx.proto 5/5 Added new proto definitions for TxStatus gRPC service to query transaction status from Celestia, replacing the broken API
crates/astria-sequencer-relayer/src/relayer/celestia_client/builder.rs 5/5 Updated imports to use BlobTxClient from celestia.core.v1.tx module and instantiate it in the builder
crates/astria-sequencer-relayer/src/relayer/celestia_client/mod.rs 4/5 Replaced GetTx with TxStatus API - removed old imports, added new blob_tx_client, and updated get_tx() to use tx_status() with new response parsing logic. Left unused block_height_from_response() function.

Sequence Diagram

sequenceDiagram
    participant Relayer as Sequencer Relayer
    participant BlobTxClient as BlobTxClient<br/>(celestia.core.v1.tx)
    participant CelestiaApp as Celestia App

    Note over Relayer,CelestiaApp: Transaction Submission Flow
    
    Relayer->>Relayer: try_prepare(blobs)
    Relayer->>CelestiaApp: broadcast_tx(blob_tx)
    CelestiaApp-->>Relayer: tx_hash
    
    Note over Relayer,CelestiaApp: Transaction Confirmation Loop (NEW API)
    
    loop Until transaction confirmed
        Relayer->>BlobTxClient: tx_status(TxStatusRequest)
        Note right of BlobTxClient: Uses celestia.core.v1.tx.Tx/TxStatus<br/>(replaces old GetTx API)
        BlobTxClient->>CelestiaApp: gRPC: TxStatus
        
        alt Transaction committed
            CelestiaApp-->>BlobTxClient: TxStatusResponse<br/>(status="COMMITTED", height>0)
            BlobTxClient-->>Relayer: Ok(Some(height))
        else Transaction pending
            CelestiaApp-->>BlobTxClient: Status: NotFound
            BlobTxClient-->>Relayer: Ok(None)
            Relayer->>Relayer: sleep and retry
        else Transaction failed
            CelestiaApp-->>BlobTxClient: TxStatusResponse<br/>(execution_code!=0)
            BlobTxClient-->>Relayer: Err(GetTxResponseErrorCode)
        end
    end
    
    Relayer->>Relayer: confirm_submission() returns height
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. crates/astria-sequencer-relayer/src/relayer/celestia_client/mod.rs, line 559-601 (link)

    style: Unused function - block_height_from_response() is now dead code after switching from GetTx to TxStatus API. Consider removing it.

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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

Labels

docker-build used to trigger docker builds on PRs proto pertaining to the Astria Protobuf spec sequencer-relayer pertaining to the astria-sequencer-relayer crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants