Conversation
📝 WalkthroughWalkthroughIntroduces helpers to read/write 8-byte little-endian uint64 metadata, refactors DA/header/data/genesis height reads/writes to use them, switches tests from Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 1m 33s —— View job Code Review: chore: minor deduplication
SummaryThis is a clean, focused deduplication PR. The main changes are:
CodeRabbit Comments Already Addressed ✅Both CodeRabbit review comments appear to have already been addressed in the final version of this PR:
Issues Found1. func (s *Submitter) initializeDAIncludedHeight(ctx context.Context) error {
if height, err := s.store.GetMetadata(ctx, store.DAIncludedHeightKey); err == nil && len(height) == 8 {
s.SetDAIncludedHeight(binary.LittleEndian.Uint64(height))
}
return nil
}This function silently swallows both backend errors and malformed-length values, never returning an error and always returning 2. ds, err := store.NewTestInMemoryKVStore()
require.NoError(t, err)
store := store.New(ds)All other tests in this PR were migrated to Minor Observations (Non-Blocking)
VerdictThe core logic changes are correct and well-structured. The two CodeRabbit findings are already addressed. The only actionable items are the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
==========================================
- Coverage 61.12% 61.05% -0.08%
==========================================
Files 113 114 +1
Lines 11676 11687 +11
==========================================
- Hits 7137 7135 -2
- Misses 3737 3746 +9
- Partials 802 806 +4
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/cache/manager.go`:
- Around line 390-405: getMetadataUint64 currently collapses backend errors and
malformed values into a missing result; change its signature to return (uint64,
bool, error) so it returns the raw error when st.GetMetadata fails or when
len(b) != 8 (e.g., return an fmt.Errorf describing the malformed length), and
only use ok==false for the explicit not-found case; then update
initDAHeightFromStore to call value, ok, err := getMetadataUint64(ctx, m.store,
store.DAIncludedHeightKey), handle/log err distinctly (e.g.,
m.logger.Error/return) and only treat the not-found (ok==false) path as a
harmless skip, preserving transient read failures for diagnostics.
In `@block/internal/submitting/submitter.go`:
- Around line 357-359: The DA included height is advanced and the per-height
cache is deleted even when putUint64Metadata(s.ctx, s.store,
store.DAIncludedHeightKey, nextHeight) fails; modify the flow so you only update
s.daIncludedHeight and remove the per-height cache after putUint64Metadata
returns nil. Concretely, check the error from putUint64Metadata and on non-nil
return/propagate the error (or retry) without mutating s.daIncludedHeight or
performing the per-height cache deletion; only after a successful
putUint64Metadata call set s.daIncludedHeight = nextHeight and then delete the
per-height cache. Ensure you log the failure as you do now via
s.logger.Error().Err(err)... but avoid dropping in-memory state when persistence
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c63884b7-8812-49b2-aa3e-9a1c928062f8
📒 Files selected for processing (5)
block/internal/cache/manager.goblock/internal/cache/manager_test.goblock/internal/cache/pending_data_test.goblock/internal/cache/pending_headers_test.goblock/internal/submitting/submitter.go
…g state Address PR review feedback: - getMetadataUint64 returns (uint64, bool, error) to distinguish missing keys from backend failures - processDAInclusionLoop persists DAIncludedHeightKey before advancing in-memory state to prevent cache deletion on persist failure - Expose store.ErrNotFound and store.IsNotFound for clean sentinel checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
block/internal/submitting/submitter.go (1)
410-414: Consider aligninginitializeDAIncludedHeightwith the new error handling pattern.This method uses the old pattern (direct
GetMetadatacall without explicit not-found checking viastore.IsNotFound). While it works becauseerr == nilshort-circuits the condition, it's inconsistent with the newgetMetadataUint64pattern inmanager.go.For consistency, consider using the same approach or extracting a shared helper. However, this is a minor inconsistency and not a functional issue.
♻️ Optional: Align with new pattern
// initializeDAIncludedHeight loads the DA included height from store func (s *Submitter) initializeDAIncludedHeight(ctx context.Context) error { - if height, err := s.store.GetMetadata(ctx, store.DAIncludedHeightKey); err == nil && len(height) == 8 { - s.SetDAIncludedHeight(binary.LittleEndian.Uint64(height)) + height, err := s.store.GetMetadata(ctx, store.DAIncludedHeightKey) + if err != nil { + if !store.IsNotFound(err) { + return fmt.Errorf("failed to load DA included height: %w", err) + } + return nil // key absent, start from 0 + } + if len(height) == 8 { + s.SetDAIncludedHeight(binary.LittleEndian.Uint64(height)) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/submitting/submitter.go` around lines 410 - 414, initializeDAIncludedHeight currently calls s.store.GetMetadata directly and checks err==nil, which is inconsistent with the new error-handling pattern used by getMetadataUint64 in manager.go; update initializeDAIncludedHeight to reuse the same helper or explicit store.IsNotFound checks: call the shared getMetadataUint64 (or replicate its logic) to fetch store.DAIncludedHeightKey, handle a not-found via store.IsNotFound silently, log/return other errors, and only call s.SetDAIncludedHeight when a valid 8-byte value is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@block/internal/submitting/submitter.go`:
- Around line 410-414: initializeDAIncludedHeight currently calls
s.store.GetMetadata directly and checks err==nil, which is inconsistent with the
new error-handling pattern used by getMetadataUint64 in manager.go; update
initializeDAIncludedHeight to reuse the same helper or explicit store.IsNotFound
checks: call the shared getMetadataUint64 (or replicate its logic) to fetch
store.DAIncludedHeightKey, handle a not-found via store.IsNotFound silently,
log/return other errors, and only call s.SetDAIncludedHeight when a valid 8-byte
value is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d54e3667-a359-417a-8e6e-14855b50fa0b
📒 Files selected for processing (3)
block/internal/cache/manager.goblock/internal/submitting/submitter.gopkg/store/types.go
* main: chore: minor deduplication (#3139) refactor(store,cache)!: optimize cache restore as O(1) (#3134) ci: remove spamoor results from benchmark results per PR (#3138) build(deps): Bump rollup from 4.22.4 to 4.59.0 in /docs in the npm_and_yarn group across 1 directory (#3136) fix(block): fix blocktime timer usage (#3137) build(deps): bump core v1.0.0 (#3135)
2 participants
Overview
this pr attempts a simple deduplication of some functions
Summary by CodeRabbit
Refactor
Tests
Chores