[FVM] Remove legacy account status formats code #8299
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughRemoved legacy account-status formats and migration logic (v1/v2), consolidated to a single minimal packed layout ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.go📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Files:
{crypto,fvm,ledger,storage}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
{crypto,fvm,ledger,access,engine}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
{storage,ledger,execution,fvm}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)fvm/environment/accounts_status.go (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). (34)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/environment/accounts_status.go (1)
101-133: Remove dead code left over from migration logic removal.The variable
sizeChangeis initialized to0on line 102 and never modified. The entire block at lines 113-130 is unreachable dead code since the conditionsizeChange != 0will always be false.This appears to be leftover scaffolding from the removed v1/v2 migration logic. To complete the cleanup per the PR objective, remove this dead code.
🔎 Proposed fix
func accountStatusV3FromBytes(inp []byte) (accountStatusV3, []byte, error) { - sizeChange := int64(0) - if len(inp) < accountStatusSizeV3 { return accountStatusV3{}, nil, errors.NewValueErrorf(hex.EncodeToString(inp), "invalid account status size") } inp, rest := inp[:accountStatusSizeV3], inp[accountStatusSizeV3:] var as accountStatusV3 copy(as[:], inp) - if sizeChange != 0 { - used := as.StorageUsed() - - if sizeChange < 0 { - // check if the storage used is smaller than the size change - if used < uint64(-sizeChange) { - return accountStatusV3{}, nil, errors.NewValueErrorf(hex.EncodeToString(inp), "account would have negative storage used after migration") - } - - used = used - uint64(-sizeChange) - } - - if sizeChange > 0 { - used = used + uint64(sizeChange) - } - - as.SetStorageUsed(used) - } - return as, rest, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/util/ledger/migrations/storage_used_migration_test.gofvm/environment/accounts_status.gofvm/environment/accounts_status_test.go
💤 Files with no reviewable changes (2)
- cmd/util/ledger/migrations/storage_used_migration_test.go
- fvm/environment/accounts_status_test.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:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.go
⏰ 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). (36)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (2)
fvm/environment/accounts_status.go (2)
14-38: LGTM! Constants are correctly defined and well-structured.The size and index constants properly define the v3 layout:
- Field sizes match their data types (uint64 → 8 bytes, uint32 → 4 bytes)
- Index calculations are consistent with the sequential layout
accountStatusSizeV3correctly sums to 29 bytesAccountStatusMinSizeV4aliasingaccountStatusSizeV3establishes forward compatibility
62-73: LGTM! Forward-compatible version initialization.New accounts are correctly initialized with version 4 flag (
0x40) while using the v3 layout. This design allows future version detection without changing the current structure size. The initialization values properly set:
- Storage index to 1 (as documented)
- All counters to 0
- Version bits to 4
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
holyfuchs
force-pushed
the
holyfuchs/6462-remove-legacy-account-status
branch
from
bff30bf to
cd65ebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fvm/environment/accounts_status.go (1)
113-115: Consider adding version validation for defense-in-depth.The
Version()method exists but is not used during deserialization inaccountStatusV3FromBytes. Adding a version check would provide fail-fast behavior if legacy data is accidentally encountered, rather than silently misinterpreting it.Proposed defensive version check
Add version validation in
accountStatusV3FromBytes:func accountStatusV3FromBytes(inp []byte) (accountStatusV3, []byte, error) { if len(inp) < accountStatusSizeV3 { return accountStatusV3{}, nil, errors.NewValueErrorf(hex.EncodeToString(inp), "invalid account status size") } inp, rest := inp[:accountStatusSizeV3], inp[accountStatusSizeV3:] var as accountStatusV3 copy(as[:], inp) + + // Validate version is v3+ (version >= 3, or v4+ with 0x40 flag) + version := as.Version() + if version != 0 && version < 3 { + return accountStatusV3{}, nil, errors.NewValueErrorf(hex.EncodeToString(inp), "unsupported account status version %d (expected v3+)", version) + } return as, rest, nil }Note: This assumes version 0 with flag 0x40 represents v4, and versions 1-2 are legacy formats that should be rejected.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/util/ledger/migrations/storage_used_migration_test.gofvm/environment/accounts_status.gofvm/environment/accounts_status_test.go
💤 Files with no reviewable changes (2)
- cmd/util/ledger/migrations/storage_used_migration_test.go
- fvm/environment/accounts_status_test.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:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.go
⏰ 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 (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (2)
fvm/environment/accounts_status.go (2)
27-38: Verify that v4+ constants are intentionally defined but largely unused.The v4+ layout constants (flagIndex, storageUsedStartIndex, etc.) and version-related constants (versionMask, accountStatusV4DefaultVersionAndFlag) are defined, but the code primarily operates on
accountStatusV3structure. OnlyNewAccountStatususesaccountStatusV4DefaultVersionAndFlagfor initialization.Ensure this is intentional—either these constants are reserved for future migration work, or there's missing v4 implementation code.
#!/bin/bash # Description: Search for usage of v4 constants to verify they're not orphaned echo "=== Searching for v4 constant usage ===" rg -n "AccountStatusMinSizeV4|accountStatusV4DefaultVersionAndFlag|flagIndex|storageUsedStartIndex|storageIndexStartIndex|accountPublicKeyCountsStartIndex|addressIdCounterStartIndex" --type=go -C2 echo -e "\n=== Searching for any v4 or version 4 references ===" rg -n "v4|version.?4" --type=go fvm/environment/ -C2
101-111: No critical issue - this is correct migration design, not a regression.The commit "[FVM] Remove legacy account status formats code" intentionally eliminated v1/v2 support. The
accountStatusV3FromBytesfunction assumes all input is v4+ format by design, not by accident. This is guaranteed by the migration pipeline, which explicitly sets all accounts to version 4 (0x40) before they enter production. The validation code inaccount_key_deduplication_migration_validation.go(line 116-119) enforces that only v4 accounts exist post-migration. All tests confirm accounts carry version 4. Version validation inaccountStatusV3FromBytesis unnecessary because the deserialization function is never called on pre-migrated data—the 29-byte size check is sufficient for format compatibility.Likely an incorrect or invalid review comment.
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Looks good. Just had one comment about naming.
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Nice! I left a comment about renaming accountStatusSize, etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fvm/environment/accounts_status.go (1)
51-51: Consider renaming to remove version suffix.Now that legacy formats have been removed, the
V3suffix inaccountStatusV3is no longer necessary and may cause confusion (especially since v4 is the current format peraccountStatusV4DefaultVersionAndFlag). Consider renaming toaccountStatusalong with related function names likeaccountStatusV3FromBytes.Based on past review comments suggesting similar naming cleanup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fvm/environment/accounts_status.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:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.go
⏰ 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 (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (2)
fvm/environment/accounts_status.go (2)
15-37: LGTM: Constants are well-structured and self-documenting.The refactoring of size constants into individual components and the computed
accountStatusMinSizeimproves maintainability and clarity. The layout indices correctly derive from the size constants, preventing potential misalignment issues.
99-109: The migration code removal is intentional and was completed during Crescendo spork.The on-the-fly migration code that handled v1→v2→v3 format conversions was removed on Sep 4, 2025 as a planned cleanup. The TODO comments in the previous version explicitly indicated the code would be removed after a full migration during Crescendo. The current implementation assumes all accounts have been migrated to V3+ format and will fail with "invalid account status size" for any legacy formats.
This removal is safe: there are no remaining constants for accountStatusSizeV1 or accountStatusSizeV2 in the codebase, all tests expect version 4, and the code has been deployed for 4+ months without errors indicating the migration was successfully completed.
holyfuchs
force-pushed
the
holyfuchs/6462-remove-legacy-account-status
branch
from
23a9579 to
d35512f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @fvm/environment/accounts_status.go:
- Around line 101-111: The function accountStatusV3FromBytes should return a
clearer error and validate the embedded version: when len(inp) <
accountStatusMinSize (29) return an error that includes the actual length and
the expected minimum (29) instead of just hex data; after slicing/copying into
accountStatusV3, verify the version nibble matches v4 (use the Version() method
on accountStatusV3 or check the upper 4 bits of the first byte == 0x4) and
return a descriptive error if it does not; keep using accountStatusMinSize and
accountStatusV3 identifiers so changes are localized.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fvm/environment/accounts_status.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:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.go
⏰ 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: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (4)
fvm/environment/accounts_status.go (4)
15-25: LGTM! Clear and maintainable size constant definitions.The refactoring to explicit component sizes with a computed
accountStatusMinSizeimproves readability and maintainability. The calculation is correct (1+8+8+4+8 = 29 bytes).
53-53: LGTM! Type definition consistently updated.The type definition correctly references the new
accountStatusMinSizeconstant, maintaining consistency with the refactored size constants. Keeping theV3designation aligns with the maintainer's decision to preserve version separation for potential future migrations.
62-73: LGTM! Accessor methods correctly use new layout constants.The
NewAccountStatusfunction properly initializes withaccountStatusV4DefaultVersionAndFlag, and all accessor methods (SetStorageUsed,StorageUsed,SetStorageIndex,SlabIndex,SetAccountPublicKeyCount,AccountPublicKeyCount,SetAccountIdCounter,AccountIdCounter) correctly reference the new index constants. The logic remains functionally equivalent to the previous implementation.Also applies to: 126-165
27-39: No action required. The migration code incmd/util/ledger/migrations/account_key_deduplication_migration_validation.gois fully compatible with theAccountStatusMinSizeV4constant. It uses the constant correctly at lines 126, 127, 135, and 139 for size validation and metadata offset calculations. The constant's reference toaccountStatusMinSize(25 bytes) is properly defined and matches the expected behavior.
…2-remove-legacy-account-status
…2-remove-legacy-account-status
holyfuchs
force-pushed
the
holyfuchs/6462-remove-legacy-account-status
branch
from
b9efcdc to
27f321a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @fvm/environment/accounts_status.go:
- Around line 99-109: The function accountStatusV3FromBytes actually parses the
V4 format; rename it to accountStatusV4FromBytes and update internal identifiers
(e.g., asv3 -> asv4, any comments mentioning V3) to reflect V4, then update all
call sites to use accountStatusV4FromBytes so behavior (size check against
AccountStatusMinSize and return values) remains unchanged; ensure unit tests and
any documentation referencing accountStatusV3FromBytes are updated accordingly.
🧹 Nitpick comments (1)
fvm/environment/accounts_status.go (1)
86-97: Inconsistent naming: function and comment reference "V3" after consolidation.While the implementation correctly uses
accountStatusPacked, there are naming inconsistencies:
- Line 87: The called function is still named
accountStatusV3FromBytes- Line 92: Comment still mentions
accountStatusV3These should be updated to reflect the new naming scheme for consistency.
♻️ Suggested refactor for naming consistency
Update the function name:
-func AccountStatusFromBytes(inp []byte) (*AccountStatus, error) { - asv3, rest, err := accountStatusV3FromBytes(inp) +func AccountStatusFromBytes(inp []byte) (*AccountStatus, error) { + asPacked, rest, err := accountStatusPackedFromBytes(inp) if err != nil { return nil, err } - // NOTE: both accountStatusV3 and keyMetadataBytes are copies. + // NOTE: both accountStatusPacked and keyMetadataBytes are copies. return &AccountStatus{ - accountStatusPacked: asv3, + accountStatusPacked: asPacked, keyMetadataBytes: append([]byte(nil), rest...), }, nil }Also update the function definition at line 99:
-func accountStatusV3FromBytes(inp []byte) (accountStatusPacked, []byte, error) { +func accountStatusPackedFromBytes(inp []byte) (accountStatusPacked, []byte, error) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/util/cmd/check-storage/account_key_validation.gofvm/environment/accounts_status.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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:
cmd/util/cmd/check-storage/account_key_validation.gofvm/environment/accounts_status.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
cmd/util/cmd/check-storage/account_key_validation.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.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 {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/environment/accounts_status.go
🧬 Code graph analysis (1)
cmd/util/cmd/check-storage/account_key_validation.go (2)
fvm/environment/accounts_status.go (1)
AccountStatusMinSize(21-25)fvm/environment/account-key-metadata/metadata.go (1)
DecodeKeyMetadata(371-407)
⏰ 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 (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (state)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (5)
fvm/environment/accounts_status.go (3)
51-51: LGTM: Type rename improves clarity.The rename from
accountStatusV3toaccountStatusPackedappropriately removes version-specific naming and better describes the binary representation.
111-163: LGTM: Receiver type updates are consistent across all methods.All method receivers have been correctly updated from
*accountStatusV3to*accountStatusPacked. The method implementations remain unchanged, ensuring backward compatibility at the API level.
36-36: The account status version target is correctly V4; no discrepancy exists.The code correctly uses V4 (
0x40) as the default account status version. TheaccountStatusV3FromBytesfunction exists for backwards compatibility to read existing V3 data during migration, not to maintain V3 as an active standard. The PR objectives referenced in the original comment ("all account statuses should have been upgraded to V3") cannot be found in the provided codebase, and the RELEASE.md contains no mention of account status version targets.Likely an incorrect or invalid review comment.
cmd/util/cmd/check-storage/account_key_validation.go (2)
125-127: LGTM: Size validation correctly updated.The constant reference has been properly updated from
AccountStatusMinSizeV4toAccountStatusMinSize, maintaining the correct validation logic for accounts with one or fewer public keys.
133-140: LGTM: Key metadata extraction correctly updated.Both references to the constant in the slice operations (lines 134 and 138) have been consistently updated. The logic correctly extracts key metadata starting after the base account status structure.
…2-remove-legacy-account-status
holyfuchs
force-pushed
the
holyfuchs/6462-remove-legacy-account-status
branch
from
27f321a to
2f159f0
Compare
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Nice! Thanks for working on this! 👍
closes: #6462
Summary by CodeRabbit
Refactor
Tests
Bug Fix
✏️ Tip: You can customize this high-level summary in your review settings.