Add account key metadata utility functions to FVM pkgs#8315
Add account key metadata utility functions to FVM pkgs#8315
Conversation
This commit copies account public key metadata utility functions from migrations package to FVM packages. These functions are used in validation programs (to check storage) and also used in unit tests for decoding and parsing. Also, this commit refactors NewKeyMetadataAppenderFromBytes() function to extract some code to functions for reuse.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds exported decoders and supporting parsing for account key metadata and batched public keys in the FVM environment: DecodeWeightAndRevokedStatuses, DecodeMappings, DecodeDigests, DecodeKeyMetadata, and DecodeBatchPublicKeys, plus associated tests and refactored internal parsing. Changes
Sequence Diagram(s)(omitted — changes are library decoding utilities without multi-component sequential interactions) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (33)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
🤖 Fix all issues with AI agents
In @fvm/environment/account_public_key_util.go:
- Line 391: Fix the typo in the error message string used when extra bytes
remain in batch public key parsing: change "batch public key data has trailiing
data (%d bytes): %x" to "batch public key data has trailing data (%d bytes): %x"
in the code that constructs the error (the string literal used in the error
return/log inside the batch public key parsing function).
In @fvm/environment/account-key-metadata/digest_test.go:
- Around line 137-141: Update the test case name string currently set to "1
digests" to the correct singular form "1 digest" in the test table entry (the
struct with fields name: "1 digests", encodedDigests: []byte{0,0,0,0,0,0,0,1},
digests: []uint64{1}); simply change the name field value to "1 digest" so the
test description is grammatically correct.
🧹 Nitpick comments (2)
fvm/environment/account_public_key_util.go (1)
359-398: Consider refactoring getRawStoredPublicKey to use DecodeBatchPublicKeys.The decoding logic in
DecodeBatchPublicKeys(lines 368-385) duplicates similar logic ingetRawStoredPublicKey(lines 263-280). Now thatDecodeBatchPublicKeysis exported,getRawStoredPublicKeycould potentially call it to decode the batch and then select the key at the desired index, reducing duplication.♻️ Example refactor
In
getRawStoredPublicKey, after retrieving the batch register value (line 254-262), you could replace the inline decoding loop (lines 263-280) with:encodedKeys, err := DecodeBatchPublicKeys(b) if err != nil { return nil, err } if keyIndexInBatch >= uint32(len(encodedKeys)) { return nil, errors.NewStoredPublicKeyNotFoundError( fmt.Sprintf("%s register doesn't have key at index %d", batchRegisterKey, keyIndexInBatch), address, storedKeyIndex) } return slices.Clone(encodedKeys[keyIndexInBatch]), nilfvm/environment/account-key-metadata/digest_test.go (1)
152-161: Consider skipping the equality assertion when an error is expected.When
tc.hasErroris true, the test still assertsrequire.Equal(t, tc.digests, decoded)on line 160. While this works (both arenil), it's cleaner to either:
- Skip the assertion when an error is expected, or
- Explicitly set
digests: nilin the error test case for clarity.♻️ Suggested improvement
t.Run(tc.name, func(t *testing.T) { decoded, err := DecodeDigests(tc.encodedDigests) if tc.hasError { require.Error(t, err) - } else { - require.NoError(t, err) + return } + require.NoError(t, err) require.Equal(t, tc.digests, decoded) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
fvm/environment/account-key-metadata/digest.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account-key-metadata/key_index_mapping_group.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/metadata.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.gofvm/environment/account_public_key_util.gofvm/environment/account_public_key_util_test.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:
fvm/environment/account-key-metadata/digest.gofvm/environment/account_public_key_util_test.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account_public_key_util.gofvm/environment/account-key-metadata/metadata.gofvm/environment/account-key-metadata/weight_and_revoked_group.gofvm/environment/account-key-metadata/key_index_mapping_group.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/account-key-metadata/digest.gofvm/environment/account_public_key_util_test.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account_public_key_util.gofvm/environment/account-key-metadata/metadata.gofvm/environment/account-key-metadata/weight_and_revoked_group.gofvm/environment/account-key-metadata/key_index_mapping_group.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.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/account-key-metadata/digest.gofvm/environment/account_public_key_util_test.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account_public_key_util.gofvm/environment/account-key-metadata/metadata.gofvm/environment/account-key-metadata/weight_and_revoked_group.gofvm/environment/account-key-metadata/key_index_mapping_group.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/account-key-metadata/digest.gofvm/environment/account_public_key_util_test.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account_public_key_util.gofvm/environment/account-key-metadata/metadata.gofvm/environment/account-key-metadata/weight_and_revoked_group.gofvm/environment/account-key-metadata/key_index_mapping_group.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.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:
fvm/environment/account_public_key_util_test.gofvm/environment/account-key-metadata/key_index_mapping_group_test.gofvm/environment/account-key-metadata/digest_test.gofvm/environment/account-key-metadata/metadata_test.gofvm/environment/account-key-metadata/weight_and_revoked_group_test.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/account-key-metadata/digest.go
🧬 Code graph analysis (6)
fvm/environment/account_public_key_util_test.go (2)
model/flow/ledger.go (2)
AccountPublicKey0RegisterID(65-70)AccountBatchPublicKeyRegisterID(72-77)fvm/environment/account_public_key_util.go (2)
DecodeBatchPublicKeys(360-398)MaxPublicKeyCountInBatch(14-14)
fvm/environment/account_public_key_util.go (2)
fvm/errors/errors.go (1)
NewCodedFailuref(319-330)fvm/errors/codes.go (1)
FailureCodeBatchPublicKeyDecodingFailure(40-40)
fvm/environment/account-key-metadata/metadata.go (2)
fvm/errors/account_key.go (2)
NewKeyMetadataEmptyError(7-12)NewKeyMetadataTrailingDataError(40-47)fvm/environment/account-key-metadata/weight_and_revoked_group.go (2)
WeightAndRevokedStatus(326-329)DecodeWeightAndRevokedStatuses(332-358)
fvm/environment/account-key-metadata/weight_and_revoked_group.go (1)
fvm/errors/account_key.go (1)
NewKeyMetadataUnexpectedLengthError(28-36)
fvm/environment/account-key-metadata/metadata_test.go (2)
fvm/environment/account-key-metadata/weight_and_revoked_group.go (1)
WeightAndRevokedStatus(326-329)fvm/environment/account-key-metadata/metadata.go (1)
DecodeKeyMetadata(371-407)
fvm/environment/account-key-metadata/weight_and_revoked_group_test.go (1)
fvm/environment/account-key-metadata/weight_and_revoked_group.go (2)
WeightAndRevokedStatus(326-329)DecodeWeightAndRevokedStatuses(332-358)
⏰ 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)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (module/dkg)
- 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/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (13)
fvm/environment/account_public_key_util.go (1)
357-398: LGTM! Well-structured utility function.The
DecodeBatchPublicKeysfunction correctly handles edge cases (empty input, boundary validation, trailing data detection) and provides clear error messages. The implementation aligns well with the existing batch public key encoding pattern.fvm/environment/account_public_key_util_test.go (1)
241-263: LGTM! Comprehensive test coverage for DecodeBatchPublicKeys.The test additions effectively verify the new
DecodeBatchPublicKeysutility across multiple scenarios:
- Single batch with 2 keys (including nil placeholder)
- Full batch at maximum capacity
- Multiple batches
Each test correctly builds expected encoded keys and validates the decode operation returns the expected byte slices. The encode/decode roundtrip verification provides strong confidence in the implementation.
Also applies to: 273-306, 317-360
fvm/environment/account-key-metadata/digest.go (1)
85-104: LGTM!The
DecodeDigestsfunction is well-implemented:
- Proper length validation against
digestSize- Efficient slice pre-allocation with correct capacity
- Safe byte slicing due to length validation
- Correctly handles nil/empty input by returning an empty slice
fvm/environment/account-key-metadata/metadata_test.go (1)
645-778: LGTM!The
TestDecodeKeyMetadatafunction provides comprehensive test coverage for the newDecodeKeyMetadataAPI:
- Tests both deduplicated and non-deduplicated paths
- Verifies all returned components (weight/revoked statuses, mappings, digests, and start indices)
- Test data aligns with existing
TestGetKeyMetadatacases for consistency- Properly handles
nilmappings for non-deduplicated casesfvm/environment/account-key-metadata/key_index_mapping_group_test.go (2)
118-123: LGTM!Good addition of the
DecodeMappingsround-trip verification to existing tests, ensuring the decoder is consistent with the encoder.
143-149: No action required. The project requires Go 1.25.0 (from go.mod), which fully supports thefor i := range len(m)syntax introduced in Go 1.22.fvm/environment/account-key-metadata/key_index_mapping_group.go (1)
221-250: LGTM!The
DecodeMappingsfunction correctly decodes mapping groups:
- Proper length validation against
mappingGroupSize- Correctly handles both consecutive and non-consecutive groups
- Logic is consistent with the existing
getStoredKeyIndexFromMappingsfunctionNote: The initial capacity
len(b)/mappingGroupSizeis a lower bound since RLE-encoded groups expand to multiple entries. This is acceptable asappendwill grow the slice as needed.fvm/environment/account-key-metadata/weight_and_revoked_group.go (1)
325-358: LGTM!The new
WeightAndRevokedStatusstruct andDecodeWeightAndRevokedStatusesfunction are well-implemented:
- Clean public API that mirrors the internal representation
- Proper length validation using
weightAndRevokedStatusGroupSize- Decoding logic is consistent with existing
getWeightAndRevokedStatus- Reuses existing parsing functions (
parseRunLength,parseWeightAndRevokedStatus)fvm/environment/account-key-metadata/weight_and_revoked_group_test.go (2)
35-42: LGTM!The test data structures have been correctly updated to use the new public
WeightAndRevokedStatustype with its exported fieldsWeightandRevoked.
113-117: Good addition of round-trip verification.Adding
DecodeWeightAndRevokedStatusesverification ensures the decoder produces results consistent with the encoder, providing end-to-end validation.fvm/environment/account-key-metadata/metadata.go (3)
138-155: LGTM! Clean refactoring that improves code reusability.The extraction of parsing logic into
parseKeyMetadata()reduces duplication and improves maintainability. The use ofslices.Clone()appropriately prevents mutation issues as documented in the function comment.
157-199: LGTM! Well-structured parsing with comprehensive validation.The function properly validates input (empty check at lines 165-168, trailing data check at lines 190-196) and handles errors consistently. The sequential parsing with early returns on error is clear and correct.
367-407: Code implementation is correct and all dependencies are in place.Both
DecodeMappings()andDecodeDigests()functions are properly defined in the codebase (key_index_mapping_group.goanddigest.gorespectively), with appropriate error handling. TheDecodeKeyMetadata()function correctly coordinates these lower-level decoders with clean error handling and named returns.
fxamacker
deleted the
fxamacker/move-public-key-metadata-util-funcs-to-fvm
branch
4 participants
Updates #8314 #7573
This PR copies account public key metadata utility functions from migrations package to FVM packages.
These functions are used in validation programs (to check storage) and also used in unit tests for decoding and parsing.
Also, this PR refactors
NewKeyMetadataAppenderFromBytes()function to extract some code to functions for reuse.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.