Signature aggregation API fixes for small items #393
Conversation
before this, it was being rendered as: INFO [07-29|19:29:09.151] Starting the signature aggregator with config at :%s /home/gene/tmp/signature-aggregator-config.json61842304=<nil> LOG_ERROR="Normalized odd number of arguments by adding nil"
"Starting the signature-aggregator executable" message already exists elsewhere. This differentiates this log entry from that one.
and other error handling cleanup
feuGeneA
force-pushed
the
signature-aggregation-api-issue-381
branch
from
03c78ac to
079ff03
Compare
feuGeneA
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
signature-aggregator/api/api.go
Outdated
| } else if *req.QuorumNum >= 0 || *req.QuorumNum > 100 { | ||
| logger.Warn("Invalid quorum number", zap.Uint64("quorum-num", *req.QuorumNum)) | ||
| http.Error(w, "invalid quorum number", http.StatusBadRequest) | ||
| } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { |
There was a problem hiding this comment.
This was my initial mistake but this if statement is wrong and will return true for any QuorumNum supplied since any uint is >=0. It was originally meant to be <=0 but this is uint64 so that shouldn't be possible. Additionally since it's defined as 0-100 we should be safe to use uint8 here?
There was a problem hiding this comment.
addressed in da09584
signature-aggregator/api/api.go
Outdated
| } else if *req.QuorumNum >= 0 || *req.QuorumNum > 100 { | ||
| logger.Warn("Invalid quorum number", zap.Uint64("quorum-num", *req.QuorumNum)) | ||
| http.Error(w, "invalid quorum number", http.StatusBadRequest) | ||
| } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { |
There was a problem hiding this comment.
| } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { | |
| } else if req.QuorumNum <= 0 || req.QuorumNum > 100 { |
There was a problem hiding this comment.
The first portion shouldn't be necessary since it's uint right now either
my suggestion would be
| } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { | |
| } else if req.QuorumNum > 100 { |
There was a problem hiding this comment.
addressed in da09584
signature-aggregator/api/api.go
Outdated
| } else { | ||
| quorumNum = *req.QuorumNum | ||
| quorumNum = req.QuorumNum | ||
| } |
There was a problem hiding this comment.
else isn't needed here if the if returns early
There was a problem hiding this comment.
addressed in addressed in da09584
| QuorumNum *uint64 `json:"quorum-num"` | ||
| QuorumNum uint64 `json:"quorum-num"` |
There was a problem hiding this comment.
Does the code still default to 67 if omitted now? (I would assume this is if we supplied 0)
There was a problem hiding this comment.
Yes the default is applied in that quorum check if we were discussing elsewhere here.
addresses review comments #393 (comment) and #393 (comment) i went ahead and just removed the `>=0` case (which, as discussed, should really be `<=0`) since we're already checking `==0` in the previous branch, and since it can't be `<0` since it's an unsigned value.
meaghanfitzgerald
added
the
Warp Signature API
Why this should be merged
Because it fixes #381
How this was tested
With the tests in the code base