Add functionality for Protocol Buffers stable marshaling #588
Conversation
cthulhu-rider
requested review from
roman-khimov,
smallhive and
carpawell
as code owners
cthulhu-rider
force-pushed
the
feature/protobuf-encoding
branch
from
2f083ca
to
74807af
Compare
cthulhu-rider
force-pushed
the
feature/protobuf-encoding
branch
2 times, most recently
from
92adf36
to
af67961
Compare
internal/proto/encoding.go
Outdated
| // MarshalBytes encodes 'bytes' or 'string' protobuf field with given number and | ||
| // value into b and returns the number of bytes written. If the buffer is too | ||
| // small, MarshalBytes will panic. | ||
| func MarshalBytes[T Bytes](b []byte, num protowire.Number, v T) int { |
There was a problem hiding this comment.
i think i would have failed to find string here and it would have confused me
There was a problem hiding this comment.
name *BytesOrString or add 4 more functions (incl. repeated)?
There was a problem hiding this comment.
imo the clearest solution is more funcs (why 4?), but i understand that it may look like a waste of code
There was a problem hiding this comment.
SizeString, MarshalString, SizeRepeatedString, MarshalRepeatedString
internal/proto/encoding.go
Outdated
| // MarshalDouble encodes 'double' protobuf field with given number and value | ||
| // into b and returns the number of bytes written. If the buffer is too small, | ||
| // MarshalDouble will panic. | ||
| func MarshalDouble(b []byte, num protowire.Number, v float64) int { |
There was a problem hiding this comment.
why is it Double? api-go has it as float64 as i remember. i expect this package to be a translator b/w "go language" and "profobuff language"
There was a problem hiding this comment.
these are encoders of protobuf fields, so double
There was a problem hiding this comment.
they are marshallers of the go's types into some raw data, it is MarshalDouble(... v float64), why? it was like this before and i agree with it
There was a problem hiding this comment.
not some raw data but protobuf. Any bool, proto.Integer or float32 can be marshalled into double, and float64 - into uint64 or fixed64 or even bytes. With this, Double wins
what i can suggest is to call it MarshalToDouble - would this reduce ur doubts?
There was a problem hiding this comment.
Any bool, proto.Integer or float32 can be marshalled into double, and float64 - into uint64 or fixed64 or even bytes
do we marshal or plan to marshal bool, etc. into double? all i want is to have it as clear as possible. i expect a go struct that stores fields for double to have it float64. anyway, you do not call it MarshalFloat64ToDouble, so you do not plan to marshal anything except float64 (me neither), why isn't it MarshalFloat64 (a func that chooses the most appropriate container (double) for the provided go type)?
There was a problem hiding this comment.
MarshalToY is a good compromise for me that i can accept. but i have to mention that is not a flexible approach when we are talking about two different types (e.g float64 and float32 to double)
There was a problem hiding this comment.
im talking about Go->protobuf uint64->fixed64 and uint64->uint64
btw, is it a real example? do we have it? bad coding if you store smth in uint64 and do not know what proto container will be chosen (mb uint64, mb fixed64), imo
There was a problem hiding this comment.
MarshalToY is a good compromise for me that i can accept
so it's decided
not a flexible approach when we are talking about two different types (e.g float64 and float32 to double)
normally, generics will help. Otherwise, we'll reconsider naming pattern
cthulhu-rider
force-pushed
the
feature/protobuf-encoding
branch
from
af67961
to
268eaac
Compare
This introduces `proto` package providing specific protobuf encoding - strict ascending field order. The format is used in NeoFS for cryptographic signatures requiring consistent checksums. Same lib is provided by `github.com/nspcc-dev/neofs-api-go/v2` module, but it is planned to be deprecated soon. Although all functionality is duplicated, the code is improved with generic typing that have appeared during this time. The library is intended to be used by other SDK packages, and does not yet imply external import. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider
force-pushed
the
feature/protobuf-encoding
branch
from
268eaac
to
51fa18f
Compare
#583 became a fat sketch. Starting from this, changes will come partially leaving code ambiguity while facilitating review