Skip to content

Conversation

@MicBun
Copy link
Member

@MicBun MicBun commented Oct 21, 2025

Fixes critical transaction hash encoding bugs that prevented correct hash handling in broadcast and query operations.

Changes:

  • Modified base64ToHex() utility to auto-detect hexadecimal input and return as-is, eliminating unnecessary double-encoding

Impact:

  • Transaction hashes now correctly formatted as 64-character hexadecimal strings
  • Transaction queries no longer fail with "invalid hash length" errors

Business Value:
Enables reliable transaction tracking and attestation workflows by ensuring transaction identifiers are correctly formatted throughout the system.

resolves: https://github.com/trufnetwork/truf-network/issues/1274

Summary by CodeRabbit

  • Refactor
    • Improved handling of hex and base64 inputs for faster processing and more reliable validation when hex is provided.
  • Bug Fixes
    • Tx identifiers are now sent using the expected hexadecimal format, reducing encoding-related errors when communicating with nodes.

@MicBun MicBun requested a review from outerlook October 21, 2025 16:06
@MicBun MicBun self-assigned this Oct 21, 2025
@holdex
Copy link

holdex bot commented Oct 21, 2025

Time Submission Status

Member Status Time Action Last Update
MicBun ✅ Submitted 5h Update time Oct 21, 2025, 4:54 PM
@outerlook ❌ Missing - ⚠️ Submit time -

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

A hex-validation helper was added and the base64-to-hex conversion now short-circuits when the input is already hex. The txInfoClient RPC payload now sends the transaction hash as hexadecimal (no base64 conversion).

Changes

Cohort / File(s) Summary
Hex conversion optimization
src/utils/serial.ts
Added private isValidHex helper to validate hex strings (strips 0x, checks hex chars, enforces even length). Modified base64ToHex to return the cleaned hex immediately if input is valid hex; otherwise decode base64 to bytes then to hex. Minor doc/structure adjustments.
RPC request encoding change
src/api_client/client.ts
Updated txInfoClient to publish tx_hash as hex in the JSON-RPC request (removed hexToBase64 conversion). Added comment noting node expects hexadecimal hashes and base64 will cause node errors. No signature changes.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller
    participant Utils as base64ToHex
    participant Client as txInfoClient
    participant Node as RPC Node

    Caller->>Utils: provide input (hex or base64)
    alt Input is valid hex
        Utils-->>Caller: return cleaned hex (strip 0x)
    else Input is not hex
        Utils->>Utils: decode base64 -> bytes -> toHex
        Utils-->>Caller: return hex
    end

    Caller->>Client: call txInfoClient(tx_hash hex)
    Client->>Node: JSON-RPC request { "tx_hash": hex, ... }
    Node-->>Client: response
    Client-->>Caller: return response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I checked each nibble, quick and spry,

If hex is true, I skip the try.
A tidy hop, no needless roam —
The RPC now gets a hex straight home. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "chore: prevent double enconding already hexadecimal tx_hash" directly and accurately describes the primary changes in the changeset. The modifications to base64ToHex() in src/utils/serial.ts and the updated txInfoClient function in src/api_client/client.ts are both focused on preventing double-encoding of transaction hashes that are already in hexadecimal format. The title is concise, specific, and clearly conveys the main objective to prevent double-encoding issues, making it easy for teammates to understand the core change from the commit history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch workingAttest

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d006413 and 062e420.

📒 Files selected for processing (2)
  • src/api_client/client.ts (1 hunks)
  • src/utils/serial.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/serial.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/api_client/client.ts (1)
src/core/jsonrpc.ts (1)
  • TxQueryRequest (79-81)
🔇 Additional comments (1)
src/api_client/client.ts (1)

269-273: I'll verify the complete flow by checking the public API documentation and how the hash format flows through the codebase.

Now let me search for where the public txInfo method is called in the main codebase to verify the hash format flow:

Now let me search the codebase to check the JSDoc and test coverage for the txInfo public method:

Let me search the codebase directly to examine the JSDoc for the public txInfo method and integration tests:

Based on my verification of the code changes and codebase analysis, I can now provide the rewritten review comment:

Code change is correct—the fix properly addresses the double-encoding issue.

The modification to send tx_hash as-is in hexadecimal format to the kwil node is sound. The comment clearly explains the rationale, and the error message pattern ("expected 64, got 44") confirms the hex vs base64 length difference. The workflow is consistent: broadcastClient returns base64ToHex(r.result.tx_hash), which users then pass to txInfo().

However, consider documenting the expected hash format on the public API.

The txInfo(hash: string) method accepts a string with no type constraint or documentation indicating the expected format. This creates potential for caller confusion. Two minor improvements would enhance API clarity and robustness:

  1. Add JSDoc specifying the expected format:
/**
 * Query transaction information by hash
 * @param hash - Transaction hash in hexadecimal format (64 characters)
 * @returns Transaction information including receipt and details
 */
public async txInfo(hash: string): Promise<GenericResponse<TxInfoReceipt>> {
  1. Consider adding a light validation to fail fast with a clear error if the hash doesn't match the expected 64-character hexadecimal pattern, rather than letting the node reject it downstream.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

outerlook
outerlook previously approved these changes Oct 21, 2025
@MicBun MicBun marked this pull request as draft October 21, 2025 16:19
@MicBun MicBun merged commit bc09d65 into main Oct 21, 2025
4 of 5 checks passed
@MicBun MicBun deleted the workingAttest branch October 21, 2025 16:55
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.

3 participants