[contractstaking] improve robustness #3892
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3892 +/- ##
==========================================
+ Coverage 75.38% 75.57% +0.19%
==========================================
Files 303 328 +25
Lines 25923 27808 +1885
==========================================
+ Hits 19541 21016 +1475
- Misses 5360 5716 +356
- Partials 1022 1076 +54
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| if !errors.Is(err, db.ErrNotExist) { | ||
| return err | ||
| } | ||
| height = 0 |
There was a problem hiding this comment.
add totalBucketCount = 0 to L247 below, or remove this line, to be consistent
blockindex/contractstaking/cache.go
Outdated
| s.bucketTypeMap[id] = bt | ||
| // add new bucket type map |
There was a problem hiding this comment.
combine 2 comments into 1 line
| oldDelegate := oldBi.Delegate.String() | ||
| if oldDelegate != newDelegate { | ||
| delete(s.candidateBucketMap[oldDelegate], id) | ||
| } |
There was a problem hiding this comment.
how likely is the case old bucket needs to be deleted? if it's not too much (like < 5-10%), then I think just add 2 lines (to delete empty map) here would suffice:
if len(s.candidateBucketMap[oldDelegate]) == 0 {
delete(s.candidateBucketMap, oldDelegate)
}
There was a problem hiding this comment.
i mean, if most case is to update the same bucket, then the new code is not efficient, it is doing
delete(map, 5)
map[5] = bucket
while the first delete is wasted
There was a problem hiding this comment.
the efficiency doesn't matter much here, otherwise more if condition will harm readability
There was a problem hiding this comment.
given the same correct logic, i would say we should aim for better code
the if here indicates the need to remove old/existing bucket, it's not too excessive if i think
blockindex/contractstaking/cache.go
Outdated
| @@ -23,6 +23,7 @@ type ( | |||
| bucketTypeMap map[uint64]*BucketType // map[bucketTypeId]BucketType | |||
| propertyBucketTypeMap map[int64]map[uint64]uint64 // map[amount][duration]index | |||
| totalBucketCount uint64 // total number of buckets including burned buckets | |||
| height uint64 // current block height | |||
There was a problem hiding this comment.
add comments for the reason to put height in the cache
|
Kudos, SonarCloud Quality Gate passed!
|
Description
improve code robustness according to new comments in #3888
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: