Repository navigation
refactor(core): read headers, state updates and classes through typed buckets - #4235
danielntmd wants to merge 1 commit into
Conversation
… buckets Route the header, state update and class Get, Write and Delete accessors through their typed buckets. ClassBucket now uses the Cbor serializer, which matches how the class is stored. Bucket.GetPtr decodes into a heap value, so accessors that return a pointer do not copy it a second time.
|
Claude finished @danielntmd's task in 17m 40s —— View job Review complete
Summary: Clean, mechanical refactor. No blockers or important issues found. Notes from the review (no action needed):
|
Titlerefactor(core): read headers, state updates and classes through typed buckets PR TypeEnhancement, Tests Description
|
| Relevant files | |||||||||
|---|---|---|---|---|---|---|---|---|---|
| Enhancement |
| ||||||||
| Tests |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4235 +/- ##
==========================================
- Coverage 81.25% 81.12% -0.14%
==========================================
Files 479 479
Lines 35591 35568 -23
==========================================
- Hits 28921 28855 -66
- Misses 6664 6707 +43
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return value, err | ||
| } | ||
|
|
||
| func (b Bucket[K, V, KS, VS]) GetPtr(database db.KeyValueReader, key K) (*V, error) { |
There was a problem hiding this comment.
I think we should defer the reference to the caller, if we want to use return by value by default.
There was a problem hiding this comment.
If we want to keep only the return by value, we can drop this and accept the cost of an extra allocation for callers that return pointers.
| db.Class, | ||
| key.ClassHash, | ||
| value.Binary[DeclaredClassDefinition](), | ||
| value.Cbor[DeclaredClassDefinition](), |
There was a problem hiding this comment.
Although the existing typed bucket code was incorrect and the new code is, I think we should replace this by something like BinaryCbor.
One of the reason is because this is an implicit fxamacker feature to use MarshalBinary and wrap it as a CBOR bytestring if the type implements MarshalBinary. This means if we change the cbor library to anything that doesn't support this behaviour, it will be incorrect.
Other affected types are AggregatedBloomFiltersBucket and RunningEventFilterBucket which have the same behaviour.
There was a problem hiding this comment.
We can call MarshalBinary ourselves and let fxamacker write and read the byte slice, but there is a class read cost (2x bytes allocated on class reads) when unmarshalling into a byte slice.
Alternatively, we can either pin this behavior through the golden tests for the AggregatedBloomFiltersBucket and RunningEventFilterBucket or we can create a zero copy unmarshal call specifically for that header but that would add another API to cbor (and ~30 lines to own CBOR header parsing, which I would want to avoid).
Route the header, state update and class Get, Write and Delete accessors through their typed buckets.
Testing