-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(da): update to go-da v0.4.0; add namespace flag #1542
Conversation
WalkthroughThe recent update introduces a new configuration parameter, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.mod
is excluded by:!**/*.mod
go.sum
is excluded by:!**/*.sum
Files selected for processing (11)
- config/config.go (4 hunks)
- config/defaults.go (1 hunks)
- da/da.go (4 hunks)
- da/da_test.go (1 hunks)
- node/full.go (2 hunks)
- node/full_client_test.go (7 hunks)
- node/full_node_integration_test.go (5 hunks)
- node/full_node_test.go (1 hunks)
- node/helpers_test.go (3 hunks)
- node/node_test.go (2 hunks)
- rpc/json/helpers_test.go (2 hunks)
Additional comments: 26
node/helpers_test.go (1)
- 21-25: The implementation of hex decoding in
getMockDA
correctly handles the conversion ofMockNamespace
to a byte slice. However, ensure thatMockNamespace
is always a valid hex string to prevent runtime errors during tests.node/node_test.go (2)
- 21-21: The addition of
MockNamespace
as a global variable is appropriate for testing the namespace functionality. Ensure that its value aligns with expected formats and use cases in tests.- 50-50: The inclusion of
DANamespace: MockNamespace
in theconfig
declaration withinnewTestNode
correctly integrates the namespace into node configuration for tests. This change is consistent with the PR objectives.rpc/json/helpers_test.go (2)
- 35-35: The introduction of
MockNamespace
with the value "deadbeef" for RPC testing is appropriate. Ensure that this mock value is suitable for all test scenarios where a namespace is required.- 82-82: Updating the
NewNode
function call to includeDANamespace: MockNamespace
correctly integrates the namespace into the node configuration for RPC tests. This ensures that the namespace functionality is adequately tested in RPC scenarios.config/config.go (4)
- 22-22: The introduction of
flagDANamespace
as a constant for the namespace configuration flag is correctly implemented. This constant is essential for consistent flag usage across the configuration handling.- 41-41: Adding
DANamespace
to theNodeConfig
struct is appropriate for storing the namespace configuration. This addition is crucial for integrating the namespace functionality into the node's configuration.- 99-99: The update to
GetViperConfig
to read the namespace configuration from Viper usingflagDANamespace
is correctly implemented. This ensures that the namespace configuration can be loaded from external configuration sources.- 114-114: The addition of the namespace configuration flag to the command-line interface in
AddFlags
usingflagDANamespace
is correctly implemented. This allows users to specify the namespace configuration via command-line flags.da/da.go (4)
- 78-81: The addition of the
Namespace
field to theDAClient
struct is correctly implemented. This field is essential for storing the namespace configuration and using it in DA operations.- 126-126: Passing
dac.Namespace
as an additional parameter in theSubmit
method call is correctly implemented. This ensures that the namespace is utilized in block submission operations.- 156-156: Passing
dac.Namespace
as an additional parameter in theGetIDs
method call is correctly implemented. This ensures that the namespace is utilized in block retrieval operations.- 178-178: Passing
dac.Namespace
as an additional parameter in theGet
method call is correctly implemented. This ensures that the namespace is utilized in blob retrieval operations.node/full_node_test.go (1)
- 114-114: Modifying the
getMockDA
function call to include thet
parameter inTestInvalidBlocksIgnored
is correctly implemented. This adjustment likely improves error handling or logging within the test setup.da/da_test.go (5)
- 54-54: Adding the
ns da.Namespace
parameter to theGet
method in theMockDA
struct is correctly implemented. This change is necessary for testing the namespace functionality in data retrieval operations.- 59-59: Adding the
ns da.Namespace
parameter to theGetIDs
method in theMockDA
struct is correctly implemented. This change is necessary for testing the namespace functionality in ID retrieval operations.- 64-64: Adding the
ns da.Namespace
parameter to theCommit
method in theMockDA
struct is correctly implemented. This change is necessary for testing the namespace functionality in commit operations.- 69-75: Updating the
Submit
method in theMockDA
struct to include thens da.Namespace
parameter and changing its return type to only include[]da.ID
and an error is correctly implemented. This aligns with the updated interface for theSubmit
method, focusing on ID return values.- 79-84: Adding the
ns da.Namespace
parameter to theGetProofs
andValidate
methods in theMockDA
struct is correctly implemented. These changes are necessary for testing the namespace functionality in proof retrieval and validation operations.node/full_node_integration_test.go (5)
- 62-62: The
MockNamespace
variable used as theDANamespace
parameter is not defined within this file. Ensure it is properly defined and accessible in the test context.- 187-187: Similar to the previous comment, verify that
MockNamespace
is appropriately defined and accessible for use as theDANamespace
parameter in theNewNode
function call.- 545-545: In the
testSingleAggregatorSingleFullNodeSingleLightNode
function, thegetMockDA
function is called but not defined within this file. Ensure that this function is correctly implemented and accessible in the test context.- 642-642: The
getMockDA
function is called again without being defined in this file. Verify its implementation and accessibility.- 710-710: Ensure that
MockNamespace
is defined and accessible for use in theNewNode
function call within thecreateNode
function.node/full_client_test.go (2)
- 181-181: The addition of
DANamespace
in theNodeConfig
struct is consistent with the PR objectives. However, verify thatMockNamespace
is appropriately defined and represents a realistic namespace value for testing.- 933-934: The configuration of
DANamespace
inNodeConfig
is appropriate for the test context. Double-check that the namespace used here does not conflict with namespaces used in parallel tests or requires specific setup in the mock environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- node/full.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- node/full.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utack
Overview
This PR updates go-da to v0.4.0; since it requires namespace to be passed to methods, a namespace flag is added.
This makes rollkit compatible with celestia-da v0.13.0 which also integrates go-da v0.4.0.
Checklist
Summary by CodeRabbit