create new codec with correct type #752
Conversation
|
|
||
| func init() { | ||
| var err error | ||
| networkCodec, err = message.NewCodec(message.BlockSyncSummary{}) |
There was a problem hiding this comment.
nit: should we name this testCodec? (or have a package we could import and use like messagetest.Codec?)
we could also have a function like newTestCodec(t), but I don't know if it is worth this change here given the other code follows the pattern with init
There was a problem hiding this comment.
hmm it could be a bit weird to export it for tests but not for prod code.
There was a problem hiding this comment.
I intentionally avoided to export it so that it won't be mistakenly imported from message pkg everytime.
| // codecWithAtomicSync is the codec manager that contains the codec for AtomicBlockSyncSummary and | ||
| // other message types that are used in the network protocol. This is to ensure that the codec | ||
| // version is consistent across all message types and includes the codec for AtomicBlockSyncSummary. | ||
| var codecWithAtomicSync codec.Manager |
There was a problem hiding this comment.
this was a little confusing to me since it's not used in the production code path, could we move it to the test file?
(should ParseFromBytes use this instead of atomic.Codec?)
There was a problem hiding this comment.
I got even more confused why and how this was not a problem and put up a UT. Seems we don't really care about if a type is registered or not in linearcodec unless we marshal/unmarshal with an interface.
Why this should be merged
Replaces error-prone sync summary type registration with a initializer that returns codec.
How this works
Codec Management Refactoring:
plugin/evm/message/block_sync_summary.go: Introduced a new codec managercodecWithBlockSyncforBlockSyncSummaryand moved related code fromsyncable.goto here.plugin/evm/message/codec.go: Added aNewCodecfunction to create codec managers for different sync summary types, replacing the previous globalCodecvariable. [1] [2]Updates to Atomic Sync Summary:
plugin/evm/atomic/sync/atomic_block_sync_summary.go: Renamed fromsyncable.goand updated to use the newCodecWithAtomicSynccodec manager. Removed the old codec initialization andSyncSummaryTypeassignment. [1] [2]Test Updates:
plugin/evm/atomic/sync/atomic_syncer_test.go: Updated to useCodecWithAtomicSync.plugin/evm/message/block_request_test.go: Updated to usecodecWithBlockSync. [1] [2]plugin/evm/message/code_request_test.go: Updated to usecodecWithBlockSync. [1] [2]plugin/evm/message/leafs_request_test.go: Updated to usecodecWithBlockSync. [1] [2]plugin/evm/message/signature_request_test.go: Updated to usecodecWithBlockSync. [1] [2] [3]Minor Changes:
plugin/evm/message/block_request.go: Simplified the variable declaration.plugin/evm/message/syncable.go: Cleaned up unused imports.How this was tested
Existing tests should cover this
Need to be documented?
No
Need to update RELEASES.md?
No