UnmarshalFrom support in codec.Codec #3335
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronbuchwald
approved these changes
Aug 28, 2024
aaronbuchwald
approved these changes
Aug 29, 2024
darioush
approved these changes
Aug 29, 2024
codec/codec.go
Outdated
| @@ -23,6 +23,7 @@ var ( | |||
| type Codec interface { | |||
| MarshalInto(interface{}, *wrappers.Packer) error | |||
| Unmarshal([]byte, interface{}) error | |||
There was a problem hiding this comment.
Would it make sense to remove Unmarshal entirely and have the codec manager do the exhaustion check?
This interface feels weird. (Not really having to do with your PR... but the existing code feels weird)
There was a problem hiding this comment.
If we'd prefer to leave that as a potential followup - I think that's reasonable too.
There was a problem hiding this comment.
I think that should be a follow up instead of being part of this PR
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: aaronbuchwald <[email protected]>
aaronbuchwald
approved these changes
Sep 4, 2024
StephenButtolph
approved these changes
Sep 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
This should be merged because it makes sense for HyperSDK to use avalanchego's default codec, but the current implementation is not compatible.
How this works
This adds an
UnmarshalFromfunction in thecodec.Codecinterface, allowing unpacking directly fromwrappers.Packerinstead of[]byte.How this was tested
In codectest.go
It was also indirectly tested in this HyperSDK PR.