-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor sign/verify message #6
base: master
Are you sure you want to change the base?
Refactor sign/verify message #6
Conversation
fyrchik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think api-go: can be replaced with a signature: in the commit header, it should designate a component in this repo (or omitted).
Also I believe it should be Verify parts IN parallel.
signature/verify.go
Outdated
| make([]byte, 0, size), | ||
| make([]byte, 0, size), | ||
| make([]byte, 0, size), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if it makes sense to allocate once and use sub-slices here. Probably not, can bite us in an unexpected way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
signature/verify.go
Outdated
| size := collection.Max(body.StableSize(), meta.StableSize(), verificationHeader.StableSize()) | ||
| buf := make([]byte, 0, size) | ||
| return verifyServiceRequestRecursive(body, meta, verificationHeader, buf) | ||
| buffers := createBuffers(size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Max was here when we had a single buffer, do we need a single size now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
util/collection/slice.go
Outdated
| func Max[T constraints.Ordered](items ...T) T { | ||
| if len(items) == 0 { | ||
| panic("failed to get max value: empty slice") | ||
| } | ||
| result := items[0] | ||
| for i := 1; i < len(items); i++ { | ||
| if items[i] > result { | ||
| result = items[i] | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be used only from 1 package, why have you decided to exprort it?
Also, it isn't needed after parallelizations (see my later comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropped
fixed |
Update go version 1.17 -> 1.18 Signed-off-by: Dmitrii Stepanov <[email protected]>
Split methods to separate files, drop redundant intefaces Signed-off-by: Dmitrii Stepanov <[email protected]>
Add benchmarks for sign and verify methods Signed-off-by: Dmitrii Stepanov <[email protected]>
Sign request/response parts in parallel Signed-off-by: Dmitrii Stepanov <[email protected]>
Verify request/response parts in parallel Signed-off-by: Dmitrii Stepanov <[email protected]>
Fix test run version Signed-off-by: Dmitrii Stepanov <[email protected]>
benchstat compare with master: