Remove code marked for removal in mainnet27 #8326
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughRemoved deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 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). (37)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @utils/unittest/fixtures.go:
- Line 2325: The fixture change made DKGIndexMap non-nil in
EpochCommitFixture(), which breaks the test that asserts
require.Nil(...DKGIndexMap); either revert the fixture to leave DKGIndexMap nil
(remove the DKGIndexMap: flow.DKGIndexMap{...} entry) or update the failing test
to expect a non-nil map by using WithPinnedField("DKGIndexMap") on the fixture
or adjusting the assertion to validate the expected mapping; alternatively, if
the intent was to populate DKG only for specific tests, keep the fixture nil and
use WithDKGFromParticipants() in those tests that need a populated DKGIndexMap.
🧹 Nitpick comments (1)
model/flow/epoch.go (1)
400-419: Validation logic is well-structured.The enforcement of non-nil
DKGIndexMapwith proper invariant checks is correctly implemented:
- Nil check guards against invalid input
- Length consistency ensures DKG participants match keys
- Index validation confirms proper bijection to
[0, n-1]One minor note: the error message on line 406 has slightly awkward phrasing where
%dprecedes the noun.Optional: Improve error message phrasing
- return nil, fmt.Errorf("number of %d Random Beacon key shares is inconsistent with number of DKG participants (len=%d)", len(untrusted.DKGParticipantKeys), len(untrusted.DKGIndexMap)) + return nil, fmt.Errorf("number of Random Beacon key shares (%d) is inconsistent with number of DKG participants (%d)", len(untrusted.DKGParticipantKeys), len(untrusted.DKGIndexMap))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/scaffold.gomodel/flow/epoch.gomodel/flow/epoch_test.gomodel/flow/service_event.gostorage/store/headers.goutils/unittest/fixtures.go
💤 Files with no reviewable changes (1)
- cmd/scaffold.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
model/flow/epoch_test.goutils/unittest/fixtures.gostorage/store/headers.gomodel/flow/service_event.gomodel/flow/epoch.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
model/flow/epoch_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
storage/store/headers.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
storage/store/headers.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to **/*_test.go : Use fixtures for realistic test data as defined in `/utils/unittest/`
Applied to files:
utils/unittest/fixtures.go
🧬 Code graph analysis (3)
model/flow/epoch_test.go (2)
model/flow/epoch.go (2)
UntrustedEpochCommit(387-387)NewEpochCommit(393-428)model/flow/dkg.go (1)
DKGIndexMap(110-110)
utils/unittest/fixtures.go (1)
model/flow/dkg.go (1)
DKGIndexMap(110-110)
model/flow/epoch.go (1)
model/flow/dkg.go (1)
DKGIndexMap(110-110)
⏰ 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). (37)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (3)
model/flow/service_event.go (1)
189-189: LGTM!Good catch on the typo fix: "ot type" → "of type" improves error message clarity.
storage/store/headers.go (1)
236-240: LGTM!Documentation cleanup for
BlockIDByViewaligns with the PR objective to remove mainnet27-related comments. The reformatted error documentation is now consistent with other methods in this file.model/flow/epoch_test.go (1)
674-686: LGTM!Good test coverage for the new nil
DKGIndexMapvalidation. SettingDKGParticipantKeysto an empty slice ensures the nil check is reached before the length mismatch check, correctly validating the intended code path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Addresses TODO comments referring to mainnet27:
flow.DKGIndexMap#6794)--pebble-dirflagBlockIDByViewSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.