Skip to content

refactor: remove full DA node type#184

Merged
chatton merged 1 commit intomainfrom
rootulp/remove-full-da-node-type
Mar 3, 2026
Merged

refactor: remove full DA node type#184
chatton merged 1 commit intomainfrom
rootulp/remove-full-da-node-type

Conversation

@rootulp
Copy link
Collaborator

@rootulp rootulp commented Mar 3, 2026

Summary

  • Remove FullNode from DANodeType constants and "full" from daNodeTypeStrings since celestia-node has deprecated and removed the celestia full subcommand
  • Remove GetFullNodes() from the DA network API
  • Update all tests to use only bridge and light nodes
  • Update comments referencing the full node type

Closes #183

Test plan

  • go build ./... passes
  • go vet ./... passes
  • Verify consensus layer NodeTypeConsensusFull and chain.FullNodes are NOT affected by this change

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Refactoring

    • Removed FullNode type from available data availability node types; framework now exclusively supports Bridge and Light nodes.
    • Refactored all test configurations and validation assertions to support Bridge and Light node topologies only.
    • Removed GetFullNodes accessor method from the data availability network API.
  • Documentation

    • Updated node configuration documentation to remove FullNode type references.

celestia-node has deprecated and removed the full node type. The celestia
full subcommand no longer exists in recent versions. Remove FullNode from
DANodeType constants, GetFullNodes() from the network API, and update all
tests to use only bridge and light nodes.

Closes #183

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 3, 2026

📝 Walkthrough

Walkthrough

Removes the FullNode DA node type across the framework due to celestia-node deprecation. Updates include removing the DANodeType constant, public GetFullNodes() method, test configurations, and refactoring test logic to exclusively use BridgeNode and LightNode types.

Changes

Cohort / File(s) Summary
Type Definitions
framework/types/node.go
Removed FullNode constant from DANodeType enum and corresponding "full" string mapping in daNodeTypeStrings.
DA Network API
framework/docker/dataavailability/network.go
Removed public GetFullNodes() method that retrieved full nodes from the network.
Configuration Documentation
framework/docker/dataavailability/node_config.go
Updated NodeType field and WithNodeType method comments to remove references to "full" node type.
Test Updates
framework/docker/da_network_test.go, framework/docker/docker_test.go
Removed fullNodeConfig setup and FullNode-related test assertions; refactored test topology to use only BridgeNode and LightNode for creation, addition, removal, and restart scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • chatton
  • mojtaba-esk
  • tty47

Poem

🐰 "The full node once stood tall and proud,
But celestia's times have changed the crowd,
Now bridge and light skip through the day,
Two nodes are just the better way!"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: removing the FullNode type from the DA node system.
Linked Issues check ✅ Passed All primary objectives from issue #183 are met: FullNode removed from DANodeType [framework/types/node.go], GetFullNodes() removed from DA network API [framework/docker/dataavailability/network.go], tests updated to use only bridge and light nodes, and full node references removed from comments.
Out of Scope Changes check ✅ Passed All changes are directly scoped to removing the FullNode DA type and its references; no unrelated modifications to consensus layer or other components are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch rootulp/remove-full-da-node-type

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.

@rootulp
Copy link
Collaborator Author

rootulp commented Mar 3, 2026

Context: celestia-node no longer has a full node.

Ref: https://github.com/celestiaorg/celestia-node?tab=readme-ov-file#run-a-node

@rootulp rootulp self-assigned this Mar 3, 2026
@rootulp rootulp marked this pull request as ready for review March 3, 2026 00:31
@rootulp rootulp requested review from chatton and tty47 as code owners March 3, 2026 00:31
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
framework/docker/da_network_test.go (1)

748-753: Guard against empty AddNodes result before indexing.

newLightNodes[0] is used immediately after require.NoError; add a length assertion first to avoid a panic on unexpected empty results.

Proposed test-hardening diff
 		newLightNodeConfig := da.NewNodeBuilder().WithNodeType(types.LightNode).Build()
 		newLightNodes, err := daNetwork.AddNodes(testCfg.Ctx, newLightNodeConfig)
 		require.NoError(t, err, "should be able to add light node back")
+		require.Len(t, newLightNodes, 1, "should return one added light node")
 		newLightNode := newLightNodes[0]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@framework/docker/da_network_test.go` around lines 748 - 753, After calling
daNetwork.AddNodes in the test, guard against an empty slice before indexing:
check the length of newLightNodes (e.g., assert len(newLightNodes) > 0) right
after require.NoError to avoid a panic when accessing newLightNodes[0]; update
the test around da.NewNodeBuilder().WithNodeType(types.LightNode).Build() /
daNetwork.AddNodes(...) to include that length assertion before assigning
newLightNode := newLightNodes[0].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@framework/docker/da_network_test.go`:
- Around line 748-753: After calling daNetwork.AddNodes in the test, guard
against an empty slice before indexing: check the length of newLightNodes (e.g.,
assert len(newLightNodes) > 0) right after require.NoError to avoid a panic when
accessing newLightNodes[0]; update the test around
da.NewNodeBuilder().WithNodeType(types.LightNode).Build() /
daNetwork.AddNodes(...) to include that length assertion before assigning
newLightNode := newLightNodes[0].

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e175641 and 1ef2bfb.

📒 Files selected for processing (5)
  • framework/docker/da_network_test.go
  • framework/docker/dataavailability/network.go
  • framework/docker/dataavailability/node_config.go
  • framework/docker/docker_test.go
  • framework/types/node.go
💤 Files with no reviewable changes (2)
  • framework/docker/dataavailability/network.go
  • framework/types/node.go

Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

🔥 🔥 🔥

Copy link
Collaborator

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

only thing to watch out for is that this would break support for running tests using older versions, potentially compatibility ones? I think it's probably not a huge deal though and it can be re-added if it becomes an issue.

@chatton chatton merged commit 227152d into main Mar 3, 2026
6 checks passed
@rootulp rootulp deleted the rootulp/remove-full-da-node-type branch March 3, 2026 16:34
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.

Remove full DA node type (celestia-node removed full subcommand)

3 participants