miner: refactor helper functions in worker.go#21044
Conversation
robert-zaremba
force-pushed
the
small-optimizations
branch
from
77c111c to
60a82f6
Compare
There was a problem hiding this comment.
This is an interface method, it should not point to anything explicit. It's up to the caller. Please delete that part.
There was a problem hiding this comment.
It's highly coupled with Wallet.SignData behaviour, notably hashing the message before signing. The current Clique implementation depends on it:
SignDatawill hash the data under the hood (without your knowledge)ecrecoverywill not hash, and if you forget about it you will have a wrong result.
There was a problem hiding this comment.
This is an interface, whether the current mplementation is a specific code is not relevant.
There was a problem hiding this comment.
I've updated the comment. I added a note about current implementation, but if you want I can remove it.
| feesWei.Add(feesWei, new(big.Int).Mul(new(big.Int).SetUint64(receipts[i].GasUsed), tx.GasPrice())) | ||
| } | ||
| return new(big.Float).Quo(new(big.Float).SetInt(feesWei), new(big.Float).SetInt(big.NewInt(params.Ether))) | ||
| } |
There was a problem hiding this comment.
This method is again a small utility code that's used solely for 1 log line. Moving it into a separate method makes things weirder because it makes the reader wonder what the importance is.
There was a problem hiding this comment.
My motivation is similar to the case with copyCurrentReceipts. This function is used in commit - the for loop is not necessary there (it is there only to create a log) and obfuscates the function, making slower to read.
In general - I would be happy, and I hope others would also benefit, if we have more such abstractions.
|
Hi @karalabe , I've responded on all comments. Any chance we can move forward? |
|
Hi, is anything still blocking this PR to be merged? |
|
Hi, can we merge this PR? |
fjl
changed the title
This reduces complexity of some lengthy functions in worker.go, making the code easier to read.
4 participants
This is a cosmetic update.
miner/worker.go- this improves readability and can also help with testing.