Skip to content

fix(pkg/da): fix json rpc types - #3200

Merged
tac0turtle merged 2 commits into
mainfrom
julien/jsonrpc
Mar 24, 2026
Merged

fix(pkg/da): fix json rpc types#3200
tac0turtle merged 2 commits into
mainfrom
julien/jsonrpc

Conversation

@julienrbrt

@julienrbrtjulienrbrt commented Mar 24, 2026

Copy link
Copy Markdown
Member

Overview

Tiny bug discovered because we've added RawHeader in #3146, does not warrant a changelog because not released.
Seeing this when on latest main.

2026-03-24T17:42:13.782Z	ERROR	rpc	go-jsonrpc@v0.10.1/client.go:506	error unmarshaling chan response: json: cannot unmarshal number into Go struct field RawHeader.header.height of type string
2026-03-24T17:42:13.824Z	ERROR	rpc	go-jsonrpc@v0.10.1/client.go:506	error unmarshaling chan response: json: cannot unmarshal number into Go struct field RawHeader.header.height of type string

https://github.com/celestiaorg/celestia-node/blob/8a784915e9fd92be22815e2aa8a50a060a7e94a2/header/header.go#L84-L86

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the data type handling for height values in JSON responses, ensuring proper numeric processing instead of text-based handling for improved data accuracy and consistency.

@github-actions

github-actionsBot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedMar 24, 2026, 6:45 PM

@coderabbitai

coderabbitaiBot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Two struct field types in the JSON RPC header module were updated from string to uint64: RawHeader.Height and Commit.Height. These changes align the JSON-decoded height fields with numeric types rather than strings, with no modifications to exported function signatures or control flow.

Changes

Cohort / File(s)Summary
JSON RPC Header Types
pkg/da/jsonrpc/header.go
Updated RawHeader.Height and Commit.Height field types from string to uint64 for numeric height representation in JSON decoding.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 Hop, hop, hooray! The heights now stand tall,
No more string-bound chains for numbers so small,
uint64 muscles flex with numeric might,
Our JSON dreams soar to numeric height! 📈✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check nameStatusExplanationResolution
Description check❓ InconclusiveThe PR description provides context about the bug, error messages, and references to related PRs, but lacks clear explanation of the fix applied.Add an explicit explanation of what changes were made (updating Height fields from string to uint64) and why this fixes the unmarshaling error.
✅ Passed checks (2 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes the main change: fixing JSON-RPC types in the pkg/da package by converting height fields from string to uint64.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch julien/jsonrpc

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.

@claude

claudeBot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @julienrbrt's task in 54s —— View job


Code Review

  • Gather context and read changed files
  • Review the changes
  • Post final review

LGTM. The fix is correct and minimal.

What changed

pkg/da/jsonrpc/header.go:23,29: RawHeader.Height and Commit.Height changed from stringuint64.

The celestia-node API encodes these nested height fields as plain JSON numbers (e.g. 123), not quoted strings ("123"). The old string type caused the unmarshaling failure reported in the PR description.

One observation worth noting

Header.Height (line 14) still uses json:"height,string,omitempty" — i.e. it expects a quoted number. This is intentional: the top-level height field on ExtendedHeader uses string encoding, while the nested header.height and commit.height fields use plain numeric encoding. The fix correctly matches the upstream celestia-node type:

// celestia-node: header.go#L84-L86// Height is encoded as a plain number in the nested header/commit,// but as a quoted string at the top-level ExtendedHeader.

Minor: no test coverage for Header deserialization

TestClient_CallsAreForwarded only exercises the blob API. There's no test that round-trips a Header through the JSON-RPC layer, which is why this regression wasn't caught earlier. Consider adding a test like:

funcTestHeader_UnmarshalJSON(t*testing.T) {
raw:=`{"header":{"chain_id":"test","height":42,"time":"..."},"commit":{"height":42},...}`varh jsonrpc.Headerrequire.NoError(t, json.Unmarshal([]byte(raw), &h))
require.Equal(t, uint64(42), h.Header.Height)
require.Equal(t, uint64(42), h.Commit.Height)
}

Not blocking — the fix itself is correct.

@codecov

codecovBot commented Mar 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.12%. Comparing base (1b73b88) to head (605ee53).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #3200 +/- ##
==========================================
+ Coverage 61.09% 61.12% +0.03% 
==========================================
Files 117 117 Lines 12082 12082 ==========================================
+ Hits 7381 7385 +4 + Misses 3873 3870 -3 + Partials 828 827 -1 
FlagCoverage Δ
combined61.12% <ø> (+0.03%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@julienrbrt
julienrbrt marked this pull request as draft March 24, 2026 17:34
@julienrbrt
julienrbrt marked this pull request as ready for review March 24, 2026 18:45
@tac0turtle
tac0turtle merged commit 9461b32 into mainMar 24, 2026
31 checks passed
@tac0turtle
tac0turtle deleted the julien/jsonrpc branch March 24, 2026 19:58
alpe added a commit that referenced this pull request Mar 25, 2026
* main:
refactor(tools/da-debug): improve da debug (#3199)
fix(pkg/da): fix json rpc types (#3200)
chore: demote warn log as debug (#3198)
build(deps): Bump the all-go group across 1 directory with 2 updates (#3191)
refactor: display block source in sync log (#3193)
chore: remove cache analyzer (#3192)
feat: add TestStatePressure benchmark for state trie stress test (#3188)
Sign up for freeto 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

@julienrbrt@tac0turtle