Enable ineffassign linter and resolve its errors#2574
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2574 +/- ##
==========================================
+ Coverage 46.90% 46.95% +0.05%
==========================================
Files 346 348 +2
Lines 55697 55716 +19
==========================================
+ Hits 26125 26164 +39
+ Misses 26615 26607 -8
+ Partials 2957 2945 -12
Continue to review full report at Codecov.
|
| if cfg.MinFee > fee { | ||
| fee = cfg.MinFee | ||
| } | ||
| var fee uint64 |
There was a problem hiding this comment.
while this code is technically correct, I think that we shouldn't do that as is.
Instead, we need to evaluate the bigger picture and (Maybe) eliminate the MinFee from the config file, as it seems redundant.
There was a problem hiding this comment.
Yes I left a comment here https://github.com/algorand/go-algorand/pull/1715/files#r671536585 about whether this was intentional or not ... I wasn't sure if this uncovered a bug or need to fix/clarify something like you're saying.
There was a problem hiding this comment.
pinging @algorandskiy for his opinion since these lines here were made ineffectual by #1715
There was a problem hiding this comment.
MinFee is used in fee() func only for fee randomization. I think it is OK to remove it completely as a separate PR. For this one removing the branch is perfectly fine.
A note about using SuggestedFee here: pingpong funds accounts between iterations to ensure they have enough algos to interact, and on a high load with app/assets they some of them might not have enough algos to submit new transactions because the pool conjunction.
There was a problem hiding this comment.
oh OK, I see so MinFee is still used in pingpong.go:WorkerState.fee(). I just wanted to make sure you realized these lines 156-159 being removed above were ineffectual (1000 is unused, and fee = cfg.MinFee is always overwritten).
tsachiherman
left a comment
tsachiherman
left a comment
There was a problem hiding this comment.
several small requests, but looks great otherwise!
| if s.cfg.CatchupVerifyTransactionSignatures() || s.cfg.CatchupVerifyApplyData() { | ||
| vb, err := s.ledger.Validate(s.ctx, *block, s.blockValidationPool) | ||
| var vb *ledger.ValidatedBlock | ||
| vb, err = s.ledger.Validate(s.ctx, *block, s.blockValidationPool) |
There was a problem hiding this comment.
@tsachiherman this is the most significant issue I found — before this change, the value of err = s.ledger.AddValidatedBlock(*vb, *cert) on line 319/320 is ineffectual, and has no impact later on line 324/325 where it checks if err != nil
tsachiherman
dismissed
their stale review
request changes applied. I'm going to wait for @algorandskiy feedback before approving & merging this one in.
| if cfg.MinFee > fee { | ||
| fee = cfg.MinFee | ||
| } | ||
| var fee uint64 |
There was a problem hiding this comment.
MinFee is used in fee() func only for fee randomization. I think it is OK to remove it completely as a separate PR. For this one removing the branch is perfectly fine.
A note about using SuggestedFee here: pingpong funds accounts between iterations to ensure they have enough algos to interact, and on a high load with app/assets they some of them might not have enough algos to submit new transactions because the pool conjunction.
4 participants
Summary
The ineffassign linter is in the default set of linters enabled by golangci-lint but wasn't added in #2523 because it was reporting issues.
It's also one of the least objectionable linters; ineffectual assignments are rarely intentional and can point out bugs related to assumptions that a variable assignment had some effect.
There were various fixes here but I'm not sure if all the changes to remove or fix ineffectual assignments were the right ones.
Test Plan
Existing tests should ideally pass.