chore: refactor submitToDA #2474
Conversation
WalkthroughThe submission logic was refactored to separate marshaling from data submission. The Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @TropicalDog17, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on refactoring the submitToDA helper function to improve its clarity and separation of concerns. By moving the data marshalling responsibility out of submitToDA and into its callers, the function becomes more focused on the submission logic itself, leading to a cleaner and potentially more reusable design.
Highlights
- Refactored
submitToDASignature: ThesubmitToDAfunction has been refactored to no longer be generic and now directly accepts[][]byte(pre-marshaled data) instead of raw items and a marshalling function. This shifts the responsibility of marshalling to the caller. - Updated
postSubmitCallback: ThepostSubmitcallback withinsubmitToDAwas updated to pass only the count of successfully submitted items (int) rather than the actual slice of submitted items. This simplifies thesubmitToDAfunction's internal state management. - Caller Adaptations: Functions like
submitHeadersToDAandsubmitDataToDAwere updated to perform the marshalling of their respective data types before callingsubmitToDA. Their post-submission logic was also adjusted to use the original item lists in conjunction with the newsubmittedCountfrom thepostSubmitcallback.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the submitToDA function to simplify its logic by moving marshaling responsibility to its callers. While the refactoring of submitToDA itself is sound, the changes in the calling functions, submitHeadersToDA and submitDataToDA, have introduced a critical bug. The logic for handling partial submissions is incorrect, which could lead to data loss as some headers or data blocks might not be submitted to the DA layer. I've provided detailed comments and suggestions to fix these issues.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2474 +/- ##
==========================================
- Coverage 74.52% 74.33% -0.20%
==========================================
Files 70 70
Lines 6537 6550 +13
==========================================
- Hits 4872 4869 -3
- Misses 1276 1287 +11
- Partials 389 394 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
can you review @alpe |
block/submitter.go
Outdated
| marshalFn func(T) ([]byte, error), | ||
| postSubmit func([]T, *coreda.ResultSubmit, float64), | ||
| marshaled [][]byte, | ||
| onSuccessfulSubmission func(int, *coreda.ResultSubmit, float64), |
block/submitter.go
Outdated
| for i, header := range headersToSubmit { | ||
| headerPb, err := header.ToProto() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to transform header to proto: %w", err) |
There was a problem hiding this comment.
personal preference: "failed to " is some prefix that can always be dropped from error messages. It is obvious that something failed when an error is returned. It also reads odd when error messages are chained and you end up with failed to ... : failed to ... : failed to ...
block/submitter.go
Outdated
|
|
||
| return submitToDA(m, ctx, marshaledHeaders, | ||
| func(submittedCount int, res *coreda.ResultSubmit, currentGasPrice float64) { | ||
| for i := 0; i < submittedCount; i++ { |
There was a problem hiding this comment.
nit: This can be simplified with Go 1.24
| for i := 0; i < submittedCount; i++ { | |
| for i := range submittedCount { |
block/submitter.go
Outdated
| func(submittedCount int, res *coreda.ResultSubmit, currentGasPrice float64) { | ||
| for i := 0; i < submittedCount; i++ { | ||
| headerIdx := submissionOffset + i | ||
| if headerIdx < len(headersToSubmit) { |
There was a problem hiding this comment.
personal preference: if you want to get the if out of the loop, you can do:
maxOffset := min(submissionOffset+submittedCount, len(headersToSubmit))
for i := submissionOffset; i < maxOffset; i++ {
m.headerCache.SetDAIncluded(headersToSubmit[i].Hash().String(), res.Height)
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the submitToDA function to be non-generic and operate on pre-marshaled data. This change simplifies the core submission logic by moving type-specific marshaling to the caller functions, submitHeadersToDA and submitDataToDA, which improves the separation of concerns. The implementation correctly handles partial submissions using a submissionOffset. My review includes suggestions to remove a couple of redundant checks to enhance code clarity.
block/submitter.go
Outdated
| if headerIdx < len(headersToSubmit) { | ||
| m.headerCache.SetDAIncluded(headersToSubmit[headerIdx].Hash().String(), res.Height) | ||
| } |
There was a problem hiding this comment.
This check if headerIdx < len(headersToSubmit) appears to be redundant. The submittedCount is derived from the number of items successfully submitted from the remaining slice in submitToDA. The length of remaining is managed to be len(headersToSubmit) - submissionOffset. Therefore, headerIdx should never exceed the bounds of headersToSubmit. Removing this unnecessary check will make the code slightly cleaner.
m.headerCache.SetDAIncluded(headersToSubmit[headerIdx].Hash().String(), res.Height)
block/submitter.go
Outdated
| if dataIdx < len(signedDataToSubmit) { | ||
| m.dataCache.SetDAIncluded(signedDataToSubmit[dataIdx].Data.DACommitment().String(), res.Height) | ||
| } |
There was a problem hiding this comment.
This check if dataIdx < len(signedDataToSubmit) appears to be redundant. The submittedCount is derived from the number of items successfully submitted from the remaining slice in submitToDA. The length of remaining is managed to be len(signedDataToSubmit) - submissionOffset. Therefore, dataIdx should never exceed the bounds of signedDataToSubmit. Removing this unnecessary check will make the code slightly cleaner.
m.dataCache.SetDAIncluded(signedDataToSubmit[dataIdx].Data.DACommitment().String(), res.Height)
|
@TropicalDog17 can you help resolve conflicts then ci should be fixed |
|
@mergify rebase |
TropicalDog17
force-pushed
the
chore/refactor-submit-toda
branch
from
74626e6 to
3c28a77
Compare
Overview
Closes #2385
Summary by CodeRabbit