Update to Cadence v1.0.0#609
Conversation
jribbink
requested review from
janezpodhostnik,
m-Peter,
peterargue,
ramtinms and
zhangchiqing
as code owners
WalkthroughThe changes in this pull request involve updating the versions of several dependencies in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Outside diff range and nitpick comments (1)
models/block.go (1)
88-89: Avoid taking the address of a local variableAssigning
fixedHash = &hashwherehashis a local variable can be unnecessary. While Go's memory management ensures safety, it's clearer to allocate a new string for better readability.Consider modifying the code as follows:
- hash := payload.Hash.String() - fixedHash = &hash + fixedHash := new(string) + *fixedHash = payload.Hash.String()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- models/block.go (2 hunks)
- models/block_test.go (2 hunks)
- services/ingestion/engine_test.go (1 hunks)
🔇 Additional comments (3)
models/block_test.go (1)
Line range hint
153-164: LGTM! Consider adding explanatory comments and reviewing other tests.The changes align well with the PR objectives of updating to Cadence v1.0.0. The switch from
decodeLegacyBlockEventtodecodeBlockEventreflects the updated event handling in the new version.Suggestions:
- Add a comment explaining the reason for the new assertion
require.NotNil(t, b.FixedHash). This will help future developers understand why this check is necessary.- Review other test cases in this file and related files to ensure consistency in using
decodeBlockEventinstead ofdecodeLegacyBlockEventwhere applicable.To ensure consistency across the codebase, let's check for any remaining usage of
decodeLegacyBlockEvent:✅ Verification successful
Verification Successful: No remaining usages of
decodeLegacyBlockEventfound.The transition to
decodeBlockEventhas been consistently applied across the codebase with no lingering references to the legacy function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of decodeLegacyBlockEvent rg 'decodeLegacyBlockEvent' --type goLength of output: 37
models/block.go (2)
76-81: Improved error handling is correctThe updated error handling provides a more descriptive error message and correctly wraps the original error using
%w.
103-103: SettingFixedHashappropriatelyAssigning
FixedHash: fixedHashensures that blocks with the legacy format have a consistent hash, maintaining integrity across different block versions.
m-Peter
force-pushed
the
auto-update-onflow-cadence-v1.0.0
branch
from
7fbd1b5 to
f97fcd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
models/block.go (1)
77-81: LGTM: Improved error handling and legacy block format handlingThe changes in the
decodeBlockEventfunction are well-implemented:
- The error handling now provides more context, which is beneficial for debugging.
- The new logic for handling legacy block formats (checking
PrevRandaoagainstzeroGethHash) ensures backward compatibility.- Setting the
FixedHashfield maintains consistency in hash calculations across different block formats.These changes align well with the PR objectives of updating to Cadence v1.0.0 and handling legacy formats.
Consider wrapping the original error in the new error message using
fmt.Errorf("... %w", err)instead offmt.Errorf("... %s", err.Error()). This preserves the original error type and allows for better error handling upstream.Also applies to: 84-90, 104-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- models/block.go (3 hunks)
- models/block_test.go (2 hunks)
- services/ingestion/engine_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- models/block_test.go
🔇 Additional comments (6)
models/block.go (4)
19-19: LGTM: Addition ofzeroGethHashvariableThe addition of the
zeroGethHashvariable is a good practice. It provides a clear, named constant for a zero hash, which enhances code readability and maintainability.
Line range hint
1-170: Summary of changes in models/block.goThe changes in this file align well with the PR objectives of updating to Cadence v1.0.0 and handling legacy formats. Key improvements include:
- Addition of
zeroGethHashfor better readability and maintainability.- Enhanced error handling in
decodeBlockEvent.- Implementation of legacy block format handling.
- Removal of deprecated
decodeLegacyBlockEventfunction andblockEventPayloadV0struct.These changes should improve the codebase's maintainability and align it with the latest versions of dependencies. However, it's crucial to verify that the removals don't introduce any regressions or break existing functionality.
Please ensure comprehensive testing is performed, particularly focusing on:
- Decoding of both new and legacy block formats.
- Proper hash calculation and consistency across different block versions.
- Backward compatibility with any systems that might still be using the old formats.
Consider adding or updating unit tests to cover these scenarios if they're not already present.
Line range hint
1-170: Verify impact of removingblockEventPayloadV0structThe removal of the
blockEventPayloadV0struct aligns with the PR objectives of updating to newer versions and simplifying the codebase. This change can improve maintainability by removing legacy code.Please ensure that this removal doesn't break any existing functionality or cause issues with data serialization/deserialization. Run the following script to check for any remaining references to this struct:
#!/bin/bash # Description: Check for any remaining references to blockEventPayloadV0 # Test: Search for any remaining references to blockEventPayloadV0 rg --type go 'blockEventPayloadV0'If the script returns any results, those areas may need to be updated or removed as well. Additionally, verify that this change doesn't affect the ability to read or process any existing data that might have been serialized using this struct.
Line range hint
1-170: Verify impact of removingdecodeLegacyBlockEventThe removal of the
decodeLegacyBlockEventfunction aligns with the PR objectives of updating to newer versions and simplifying the codebase. This change can improve maintainability by removing legacy code.Please ensure that this removal doesn't break any existing functionality. Run the following script to check for any remaining references to this function:
If the script returns any results, those areas may need to be updated or removed as well.
services/ingestion/engine_test.go (2)
553-559: Improved block creation logicThe update to use
types.NewBlockis a positive change, aligning with the latest dependency versions and providing a more comprehensive block representation, including theTotalDifficultyparameter.
553-559:⚠️ Potential issueAddress the unresolved type mismatch in
types.NewBlockcallThe previous review comment about the potential type mismatch in the last parameter of
types.NewBlockis still valid and needs to be addressed.Apply this diff to correct the parameter type:
gethBlock := types.NewBlock( gethCommon.HexToHash("0x1"), height, uint64(1337), big.NewInt(100), - gethCommon.HexToHash("0x15"), + []byte("0x15"), )
| gethBlock := types.NewBlock( | ||
| gethCommon.HexToHash("0x1"), | ||
| height, | ||
| uint64(1337), | ||
| big.NewInt(100), | ||
| gethCommon.HexToHash("0x15"), | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Missing assertions for TotalDifficulty in tests
The current tests in services/ingestion/engine_test.go do not include assertions for the TotalDifficulty property in the newBlock function. It's essential to add these assertions to ensure that the TotalDifficulty is correctly set and handled, thereby validating the updated block creation logic comprehensively.
- File:
services/ingestion/engine_test.go - Suggested Additions:
- Assertions verifying the value of
block.TotalDifficultyafter block creation. - Test cases covering different scenarios involving
TotalDifficulty.
- Assertions verifying the value of
🔗 Analysis chain
Review and update tests for new block creation logic
With the changes in the newBlock function, it's important to review the tests that use this function. Ensure that they still cover all necessary scenarios and remain valid with the updated block creation logic. Consider adding new test cases if needed to cover the new TotalDifficulty parameter and any other changes introduced by types.NewBlock.
To help with this verification, you can run the following script to identify tests that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test functions that use newBlock and might need updating
# Search for test functions that call newBlock
rg -t go --multiline '(?s)func Test.*\{.*newBlock\(' services/ingestion/engine_test.go
# Search for assertions related to block properties that might need updating
rg -t go 'assert.*block\.' services/ingestion/engine_test.go
Length of output: 15112
Script:
#!/bin/bash
# Description: Check if tests assert the TotalDifficulty property in newBlock usage
# Search for assertions on block.TotalDifficulty in engine_test.go
rg -t go 'assert.*block\.TotalDifficulty' services/ingestion/engine_test.go
Length of output: 77
3 participants
Description
Automatically update to:
Summary by CodeRabbit
New Features
Bug Fixes