Improve results handling #4177
Improve results handling #4177
Conversation
| if result, ok := (*b)[txHash][address]; ok { | ||
| return result | ||
| } | ||
| return set.NewBits() |
There was a problem hiding this comment.
The zero value of set.Bits is not safe to use afaict
| if len(txResults) == 0 { | ||
| delete(*b, txHash) | ||
| return |
There was a problem hiding this comment.
This needs to be done to work with the coreth/subnet-evm block building logic.
| type encodedBlockResults struct { | ||
| TxResults map[common.Hash]map[common.Address][]byte `serialize:"true"` | ||
| return resultsCodec.Marshal(version, encoded) | ||
| } |
There was a problem hiding this comment.
We don't need to wrap this into a type (and removing the type wrapper doesn't change the serialization format)
| common.Address{2}: set.NewBits(1, 2, 3), | ||
| }, | ||
| }, | ||
| want: []byte{ |
There was a problem hiding this comment.
Explicitly locking in the serialization format makes me a lot more comfortable to make changes here.
| {2}: { | ||
| {2}: set.NewBits(1, 2, 3), | ||
| {3}: set.NewBits(3, 2, 1), | ||
| name: "delete_tx", |
There was a problem hiding this comment.
We need this case to support Delete like it had before.
| parsed, err := ParseBlockResults(got) | ||
| require.NoError(err) | ||
| require.Equal(input, parsed) |
There was a problem hiding this comment.
I thought about moving this out to a separate test... But I felt like it was overly repetitive.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the handling of predicate results by refactoring the data structures and enhancing test coverage for serialization. The changes focus on making the API more consistent and ensuring proper handling of zero values.
- Refactored
BlockResultsfrom a struct with aTxResultsfield to a direct type alias for better API consistency - Enhanced the
Getmethod to always return a validset.Bitsinstead of a boolean-checked tuple - Improved the
Setmethod to properly handle zero values by deleting empty entries
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vms/evm/predicate/results.go | Refactored BlockResults type from struct to map alias and updated method signatures for consistency |
| vms/evm/predicate/results_test.go | Added comprehensive serialization tests and restructured existing tests to verify new behaviors |
| vms/evm/predicate/predicate_test.go | Updated test helpers to use generic set types instead of custom allowSet type |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Why this should be merged
ParseandBytes) could break existing logic.SetandGetwhere afterSeting a zero value,Getbehaves the same as if the value was never set.Sethad previously (which is required to remain compatible with the C-chain's block building logic)How this works
Updated logic to better align with coreth and improved the tests to fully verify the behaviors we expect.
How this was tested
Unit tests
Need to be documented in RELEASES.md?
No.