Unit tests #21
Unit tests #21
Conversation
minghinmatthewlam
requested review from
michaelkaplan13,
cam-schultz,
gwen917,
geoff-vball and
bernard-avalabs
as code owners
| scaledTotalWeight := new(big.Int).SetUint64(totalWeight) | ||
| scaledTotalWeight.Mul(scaledTotalWeight, new(big.Int).SetUint64(DefaultQuorumNumerator)) | ||
| scaledSigWeight := new(big.Int).Mul(accumulatedSignatureWeight, new(big.Int).SetUint64(DefaultQuorumDenominator)) | ||
| totalWeightBI := new(big.Int).SetUint64(totalWeight) |
There was a problem hiding this comment.
Is there a specific reason to introduce the new variable totalWeightBI? When compared to operating on scaledTotalWeight in place, there's an additional allocation incurred.
There was a problem hiding this comment.
I made this change to make the code a little more clear. Previously we were assigning scaledTotalWeight := new(big.Int).SetUint64(totalWeight), but it didn't actually represent the scaled total weight until the operation on the next line.
If we think the performance hit of the allocation is a big deal, we can change it back. We could also eliminate another allocation on line 48 by re-using scaledSigWeight.
There was a problem hiding this comment.
I think the extra allocation for clarify in variable naming is okay
utils/utils.go
Outdated
| @@ -58,16 +57,18 @@ func CheckStakeWeightExceedsThreshold(accumulatedSignatureWeight *big.Int, total | |||
|
|
|||
| // BigToHashSafe ensures that a bignum value is able to fit into a 32 byte buffer before converting it to a common.Hash | |||
| // Returns an error if overflow/truncation would occur by trying to perfom this operation. | |||
| // TODO is this function still needed? It works for negative numbers now. | |||
There was a problem hiding this comment.
Looking at common.BigToHash, I think we can remove this function. big.Int doesn't guarantee the internal bytes will be representable as a uint256 (though the getters will truncate to the requested type), so we may want to add redundant validity checks at the callsites.
There was a problem hiding this comment.
(though the getters will truncate to the requested type), so we may want to add redundant validity checks at the callsites.
can you clarify this, didn't get it. We aren't using BigToHashSafe anywhere anyways, so I think we can remove if we don't imagine using it.
There was a problem hiding this comment.
hmm rereading this comment, I actually prefer leaving this utils function. Right now we don't use common.BigTohash or the BigToHashSafe, but want to leave for future use, since common.BigToHash will just truncate it to 32 bytes, I think it's better to check for error.
minghinmatthewlam
requested review from
gwen917,
geoff-vball and
cam-schultz
minghinmatthewlam
dismissed stale reviews from geoff-vball, michaelkaplan13, and gwen917
via
412f95b
I'm definitely for printing out a report on our test coverage, not sure fi we'd want to require it to above a minimum threshold. Right now our % of test coverage is still low, and want to avoid tackling it all in this PR. Would be good with first printing out the test coverage %, thoughts @michaelkaplan13 ? |
Yeah I agree, it's best left for a separate PR either way. 👍 |
|
Created new issue to address test coverage: #44 |
Why this should be merged
Adding test coverage for relayer repo and fixes #11
cc @geoff-vball for prior contribution to unit tests as well.
How this works
assertwithrequirechecks, since require fails on the first error and makes it easier to debugmockgenbinary to generate mocks for interfacesHow this was tested
How this was documented
README update