Blob Serialisation #92
Conversation
nisdas
force-pushed
the
blobSerialization
branch
from
60ef8ea
to
8a8a860
Compare
sharding/client/utils.go
Outdated
| ) | ||
|
|
||
| func (c *collationbody) validateBody() bool { | ||
| return len(c) < 2^20 && len(c) > 0 |
There was a problem hiding this comment.
Inline magic numbers like this aren't understood by outside viewers. Please store in vars called maxcollationsize, mincollationsize, or another sort of construct.
There was a problem hiding this comment.
Agreed, will put a variable in place and maybe move all the constants to config.go
There was a problem hiding this comment.
I left a comment on the design as well, but this would never consider a body valid when the len(cb) == 2^20 i.e. when the collation body is exactly 1 MiB
sharding/client/utils.go
Outdated
| return len(c) < 2^20 && len(c) > 0 | ||
| } | ||
|
|
||
| func createChunks(c collationbody) { |
There was a problem hiding this comment.
I'd rather have a different name than c for this variable. It reads nicer if we call the variable collationBody or something like that as I can understand line 28 immediately upon the first read.
There was a problem hiding this comment.
It is go convention to prefer shortest variable name for the given context.
If anything, we could extend this name to cb and still understand that it is the collation body.
sharding/client/utils.go
Outdated
| return len(c) < 2^20 && len(c) > 0 | ||
| } | ||
|
|
||
| func createChunks(c collationbody) { |
There was a problem hiding this comment.
It is go convention to prefer shortest variable name for the given context.
If anything, we could extend this name to cb and still understand that it is the collation body.
sharding/client/utils.go
Outdated
| fmt.Errorf("Error %v", err) | ||
| } | ||
| if !validCollation { | ||
| fmt.Errorf("Error %v", err) |
sharding/client/utils.go
Outdated
| ) | ||
|
|
||
| func (c *collationbody) validateBody() bool { | ||
| return len(c) < 2^20 && len(c) > 0 |
There was a problem hiding this comment.
I left a comment on the design as well, but this would never consider a body valid when the len(cb) == 2^20 i.e. when the collation body is exactly 1 MiB
sharding/client/utils.go
Outdated
| type collationbody []byte | ||
|
|
||
| var ( | ||
| collationsizelimit = int64(2 ^ 20) |
There was a problem hiding this comment.
This is bitwise XOR and this evaluates to 22
|
Update on this @nisdas ? |
|
@rauljordan we had some discussion in gitter about changing this a bit. |
|
@rauljordan , Preston and I had a discussion about this on the changes to make. Which would be a bit different from how it is being implemented now. I will push something up soon |
sharding/client/utils.go
Outdated
|
|
||
| } else { | ||
| terminalIndex := int64(collationbody[(i-1)*chunkSize]) | ||
| tempbody = append(tempbody, collationbody[((i-1)*chunkSize+1):((i-1)*chunkSize+2+terminalIndex)]...) |
There was a problem hiding this comment.
Shorten line length plz. Perhaps abstract into some more vars?
sharding/client/utils.go
Outdated
|
|
||
| for i := int64(1); i <= chunksNumber; i++ { | ||
|
|
||
| if reflect.TypeOf(collationbody[(i-1)*chunkSize]) == reflect.TypeOf(indicatorByte) { |
There was a problem hiding this comment.
Shorten this if condition. Perhaps explain with some more comments?
sharding/client/utils.go
Outdated
| } | ||
| // Appends indicator bytes to terminal-chunks , and if the index of the chunk delimiter is non-zero adds it to the chunk | ||
| indicatorByte[0] = byte(terminalLength) | ||
| tempbody = append(tempbody, append(indicatorByte, cb[chunksNumber*chunkDataSize:(chunksNumber*chunkDataSize)+(terminalLength+1)]...)...) |
There was a problem hiding this comment.
This line is too long. Too much is going on here and hard to keep track as a reader.
There was a problem hiding this comment.
Yeah agreed, I will try to abstract and make the code more readable. Also will add comments to explain the serialization flow better
sharding/client/utils.go
Outdated
|
|
||
| } | ||
| indicatorByte[0] = byte(chunkDataSize) | ||
| tempbody = append(tempbody, append(indicatorByte, cb[(chunksNumber-1)*chunkDataSize:chunksNumber*chunkDataSize]...)...) |
There was a problem hiding this comment.
Too long, simplify and explain a little.
There was a problem hiding this comment.
I'm a bit worried that this util is a bit cumbersome to use.
Can we use an approach similar to the JSON marshaller?
If you recall we discussed something that looked like this
unc Marshal(v []interface{}) ([]byte, error)
func Unmarshal(data []byte, v []interface{}) error
See: https://blog.golang.org/json-and-go
I think that interface receiver approach you have taken will be hard to use and your tests might prove that too.
With the marshal method it would be trivial to use and familiar to go developers.
Objectives :
https://ethresear.ch/t/blob-serialisation/1705