refactor: firewood.Database.Update() and Commit() readability
#242
Conversation
| } | ||
|
|
||
| type proposals struct { | ||
| sync.RWMutex |
There was a problem hiding this comment.
If it's being exposed outside of the package boundary, then there's a risk of misuse leading to deadlock. But as a purely internal struct, what's the value in forcing .lock. or .mu. being added to every call site?
| _, ok := p.parent.blockHashes[common.Hash{}] | ||
| return ok | ||
| }) | ||
| if !ok { // note negative boolean |
There was a problem hiding this comment.
Why add this comment here? I think it might still be unclear what exactly this search is looking for. Maybe we still need a comment describing the hokey "oh it must be the first block after startup" thing
There was a problem hiding this comment.
Why add this comment here?
Because it's the inverse of the one above. Without it, I thought it was too easy to skim and make an incorrect assumption. Similar to this rationale.
I think it might still be unclear what exactly this search is looking for. Maybe we still need a comment describing the hokey "oh it must be the first block after startup" thing
I agree. This refactoring was predominantly about "macro" structure so the larger functions could be broken down into digestable steps before later concentrating on the contents of each one.
Why this should be merged
Simplify existing implementation.
How this works
Databasecontents to logically group similar componentsblockInfoinsideproposalas all fields effectively belong to theproposalanywayHow this was tested
N/A as base branch is still to be tested.