cmd, consensus, eth: split ethash related config to it own#15520
Conversation
karalabe
left a comment
karalabe
left a comment
There was a problem hiding this comment.
Generally good, just a minor refactor is needed.
| PowFake bool | ||
| PowTest bool | ||
| PowShared bool | ||
| } |
There was a problem hiding this comment.
Please name this struct simply Config. We're in the ethash package, so your naming would stutter ethash.EthashConfig.
| dagdir string // Data directory to store full mining datasets | ||
| dagsinmem int // Number of mining datasets to keep in memory | ||
| dagsondisk int // Number of mining datasets to keep on disk | ||
| EthashConfig |
There was a problem hiding this comment.
I don't really like when fields from multiple structs are mixed together. It would be cleaner to just have a private config field for Config.
| // Otherwise assume proof-of-work | ||
| switch { | ||
| case config.PowFake: | ||
| case config.Ethash.PowFake: |
There was a problem hiding this comment.
I think here you can swap out config from *eth.Config to *ethash.Config.
|
@karalabe Updated |
karalabe
left a comment
karalabe
left a comment
There was a problem hiding this comment.
I really like this change :) 2 minor fixups and it's ready to go in.
| Test | ||
| Fake | ||
| FullFake | ||
| ) |
There was a problem hiding this comment.
Please prefix these with Mode so that they are:
- Semantically grouped together similar to e.g. https://golang.org/pkg/net/http/#pkg-constants
- Easier to understand
ethash.ModeTestvs.ethash.Test
| d.release() | ||
| } | ||
|
|
||
| type Mode uint |
There was a problem hiding this comment.
Please document this:
// Mode defines the type and amount of PoW verification an ethash engine makes.
|
@karalabe Please take a look again! |
2 participants
No description provided.