fix: NodeIterator return error from empty trie root #119
Conversation
WalkthroughThe changes in this pull request modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MerkleTreeIterator
participant BlobFinder
Client->>MerkleTreeIterator: Initialize with root
MerkleTreeIterator->>BlobFinder: Find node blob by hash
alt Blob is empty
MerkleTreeIterator-->>Client: Return zk.EmptyNodeValue.CanonicalValue()
else Blob is not empty
MerkleTreeIterator-->>Client: Continue processing
end
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: 0
🧹 Outside diff range and nitpick comments (1)
trie/iterator_test.go (1)
664-685: LGTM! Consider improving error messages.The test cases for empty root scenarios are well-structured and properly verify the iterator behavior. However, the error messages could be more descriptive.
Consider applying this diff to improve the test failure messages:
t.Run("zk merkle tree", func(t *testing.T) { trie := NewEmptyMerkleTrie(NewZkDatabase(rawdb.NewMemoryDatabase())) it, _ := trie.NodeIterator(nil) for it.Next(true) { - t.Fail() + t.Error("iterator should not return any nodes for empty merkle tree") } if it.Error() != nil { - t.Error(it.Error()) + t.Errorf("unexpected error from empty merkle tree iterator: %v", it.Error()) } }) t.Run("zk trie", func(t *testing.T) { trie, _ := NewZkTrie(common.Hash{}, NewZkDatabase(rawdb.NewDatabase(memorydb.New()))) it, _ := trie.NodeIterator(nil) for it.Next(true) { - t.Fail() + t.Error("iterator should not return any nodes for empty zk trie") } if it.Error() != nil { - t.Error(it.Error()) + t.Errorf("unexpected error from empty zk trie iterator: %v", it.Error()) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- trie/iterator.go (1 hunks)
- trie/iterator_test.go (1 hunks)
🔇 Additional comments (1)
trie/iterator.go (1)
943-945: LGTM! Improved error handling for empty root nodes.The change correctly handles the case where the root node's blob is empty by returning early, preventing potential nil pointer dereferences in subsequent operations. This is a good defensive programming practice.
jyc228
force-pushed
the
fix/zk-iterator
branch
from
ef2fca3 to
6fbe92a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
trie/iterator_test.go (1)
664-685: Enhance test cases with more specific assertions and descriptions.While the test cases cover the basic functionality, they could be improved in several ways:
- Add test descriptions to clarify the expected behavior
- Use specific error type assertions
- Follow Go testing conventions more closely
Consider applying these improvements:
t.Run("empty root", func(t *testing.T) { t.Run("zk merkle tree", func(t *testing.T) { + t.Parallel() // Allow parallel execution trie := NewEmptyMerkleTrie(NewZkDatabase(rawdb.NewMemoryDatabase())) it, _ := trie.NodeIterator(nil) + nodeCount := 0 for it.Next(true) { - t.Fail() + nodeCount++ } + if nodeCount > 0 { + t.Errorf("expected no nodes in empty trie, got %d nodes", nodeCount) + } if it.Error() != nil { - t.Error(it.Error()) + t.Errorf("expected no error from empty trie iterator, got: %v", it.Error()) } }) t.Run("zk trie", func(t *testing.T) { + t.Parallel() // Allow parallel execution trie, _ := NewZkTrie(common.Hash{}, NewZkDatabase(rawdb.NewDatabase(memorydb.New()))) it, _ := trie.NodeIterator(nil) + nodeCount := 0 for it.Next(true) { - t.Fail() + nodeCount++ } + if nodeCount > 0 { + t.Errorf("expected no nodes in empty trie, got %d nodes", nodeCount) + } if it.Error() != nil { - t.Error(it.Error()) + t.Errorf("expected no error from empty trie iterator, got: %v", it.Error()) } }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- trie/iterator.go (1 hunks)
- trie/iterator_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- trie/iterator.go
🔇 Additional comments (1)
trie/iterator_test.go (1)
664-685: Consider adding test coverage for zkMerkleTreeNodeBlobFunctions.The current test cases verify basic iteration behavior, but considering the changes mentioned in the AI summary regarding
zkMerkleTreeNodeBlobFunctions, it would be beneficial to add specific test cases that verify the return value when the hash equalszkt.HashZero[:].Let's verify if there are any existing tests for this scenario:
Consider adding a test case that explicitly verifies the return value of
zkMerkleTreeNodeBlobFunctionswhen handling empty nodes.
| it, _ := trie.NodeIterator(nil) | ||
| for it.Next(true) { |
There was a problem hiding this comment.
Improve error handling in iterator creation.
The iterator creation error is currently being ignored with it, _ := trie.NodeIterator(nil). Consider handling this error explicitly since the PR's objective is related to NodeIterator errors.
Apply this improvement to both test cases:
- it, _ := trie.NodeIterator(nil)
+ it, err := trie.NodeIterator(nil)
+ if err != nil {
+ t.Fatalf("failed to create iterator: %v", err)
+ }Also applies to: 677-678
No description provided.