Codec & serializer - initial implementation #1
Conversation
abi/codecForCompositeTypes_list.go
Outdated
| "io" | ||
| ) | ||
|
|
||
| func (c *codec) encodeNestedList(writer io.Writer, value InputListValue) error { |
There was a problem hiding this comment.
I personally do not like splitting a large struct in more files
This could have been another independent struct that the codec struct could have extend it.
Valid for all other files containing specialized codec methods
There was a problem hiding this comment.
Indeed. Change in progress.
There was a problem hiding this comment.
Refactored, using separate structs now. Using regular composition, though, instead of go-like composition. Some codecs rely on the general codec to perform their tasks.
I've postponed the renaming of the files, so that this fix-after-review doesn't bring a big delta: #3.
abi/codecForCompositeTypes_option.go
Outdated
|
|
||
| func (c *codec) encodeNestedOption(writer io.Writer, value OptionValue) error { | ||
| if value.Value == nil { | ||
| _, err := writer.Write([]byte{0}) |
There was a problem hiding this comment.
0 & 1 could have been defined constants
There was a problem hiding this comment.
Fixed, using constants.
| return bigIntFromBytes(data, withSign) | ||
| } | ||
|
|
||
| func bigIntToBytes(value *big.Int, withSign bool) []byte { |
There was a problem hiding this comment.
I would remove the withSign bool and since this function will become 2 one-liner functions they could be dropped. Then, moving upstream, the functions from this file will be doubled (of course extracting out the common code) but we will get rid of a bool which I find ugly.
Of course, just my opinion, the code is maintainable as it is, and I'm pretty sure it works.
There was a problem hiding this comment.
Indeed, refactored.
abi/parts.go
Outdated
| } | ||
|
|
||
| func (holder *partsHolder) appendEmptyPart() { | ||
| holder.parts = append(holder.parts, []byte{}) |
There was a problem hiding this comment.
weak warning when using empty literal declaration. Switch to make([]byte, 0) ?
| err = s.serializeInputMultiValue(partsHolder, value) | ||
| case InputVariadicValues: | ||
| if i != len(inputValues)-1 { | ||
| return errors.New("variadic values must be last among input values") |
abi/shared.go
Outdated
|
|
||
| func readBytesExactly(reader io.Reader, numBytes int) ([]byte, error) { | ||
| if numBytes == 0 { | ||
| return []byte{}, nil |
| import "math/big" | ||
|
|
||
| // U8Value is a wrapper for uint8 | ||
| type U8Value struct { |
There was a problem hiding this comment.
question: would have been better if we would have avoided these wrapper structs and instead used alias types?
There was a problem hiding this comment.
Will attempt to do something like that for primitives (pending).
There was a problem hiding this comment.
For encoding, we could have passed primitive Go values - simply passed by value.
For decoding, we had two choices: either require the caller to pass a (mutable) pointer towards the (empty) primitive placeholder, or to pass a reference of a wrapper.
In JavaScript / Python etc., passing mutable pointers is not an option. And in order to have some consistency for the codecs, across multiple languages, the wrapper-based solution seemed a good option.
However, indeed, we should strive to support primitive values as much as possible, as well. I've added an issue here: #4.
Is it OK if we postpone this (somehow larger change) to a next PR?
There was a problem hiding this comment.
Sure, can be postponed. Not a blocker issue
| } | ||
|
|
||
| codec.codecForList = &codecForList{ | ||
| generalCodec: codec, |
There was a problem hiding this comment.
well, this did not turn out as expected. This is actually worse than the initial implementation, Why? We now have reciprocal pointer links which should always be avoided.
Can we move into a new baseCoded the general functionality from the codec?
Or at least, have an unexported interface instead of clear pointer.
There was a problem hiding this comment.
Applied the second suggestion (depend on the general codec by means of an interface).
There isn't a "base functionality" per-se that can be extracted to a base codec. That is, some "leaf" codecs (e.g. the one for lists of items) depend on all the other codecs at runtime (to be able to handle all types of items within the list).
Another approach (with a bigger refactoring, though) would be to attach the encoding and decoding functionality to the value structs themselves. That way, we would be able to apply the composite pattern as follows (example):
struct ListValue:
encodeNested()
for each item in the list, call item.encodeNested()
...
struct U64Value:
encodeNested():
...
list = new ListValue([new U64Value(42), new U64Value(43)])
list.encodeNested(...)
However, in this way, we would couple the dummy wrapper values with encoding / decoding logic. E.g. if we decide upon a different serialization format (though chances are slim), a separate family of structs will be required e.g. ListValueV3, OptionValueV3. This would also be possible to overcome using factories - of families of structs (with a given encoding / decoding specification), but that would somehow complicate a bit the design.
What is your opinion? All right as now, using an internal interface for generalCodec - or should we switch to another design (e.g. the one in the paragraph above)?
There was a problem hiding this comment.
Re-re-thinking about it - perhaps having moved the encoding / decoding logic in the value structs themselves, and have them implement the Encodable interface (or something like that) - with 4 functions (encode nested, encode top level, decode nested, decode top level) - does not sound that bad anymore.
There was a problem hiding this comment.
That is exactly what we have with the trie elements: leafNode, extensionNode, branchNode. The "child"/"children" node(s) is always an interface that can be either implementation
There was a problem hiding this comment.
Thanks. Added a GH issue here for this topic: #5
Initial implementation for codecs & serializer:
Usage examples are best visible in the file
abi/serializer_test.go.Not yet implemented: token identifier, arrays, tuples, counted variadic. Will be implemented in the next PR.