Validate JSON config

[adambabik]

Related to issue #149.

Changes

• exposed ValidateNodeConfig(),
• extended NodeConfig interface with Validate() method,
• added validate tag to some NodeConfig fields.

Validated fields

• field type thanks to Go JSON unmarshalling,
• required fields.

Error formatting

{
  "status": true | false,
  "message": "string",
  "field_errors": [
    {
      "paramater": "string",
      "errors": [
        {
          "message": "string"
        },
        ...
      ]
    },
    ...
  ]
}

Todo:

• better error formatting (some help from ClojureTeam is required),
• add validation for the rest of the fields.

[adambabik]

I consider this done at this point of time. I tried to make the validation more restrict but each time I was hit by some tests that allowed a given configuration. E.g. NetworkId might be arbitrary as we allow private networks, checking key paths is done later, not during loading configuration and almost all fields are optional as we provide defaults.

[MarinX]

cmd/statusd/utils.go
func testValidateNodeConfig
Please move this testing function to testing package eg. if you are testing utils create utils_test.go. We dont want to have an testing functions inside packages.

geth/params/validator.go
func NewValidator()
Can we simplify by return validator.New() ?

geth/common/types.go

• APIDetailedResponse
• APIFieldError
• APIError

Why this API prefix ? Can we simplify to something idiomatic ?
Eg. Response, Field, Error

We are using single char to point to the struct, so please, stick with that.
Ex.
func (fe APIFieldError) Error()
We can just use "f" or "e".

[adambabik]

@MarinX it would be easier to reply to comments directly attached to source files, if it's possible.

cmd/statusd/utils.go
func testValidateNodeConfig
Please move this testing function to testing package eg. if you are testing utils create utils_test.go. We dont want to have an testing functions inside packages.

You can't compile *_test.go files with cgo support and that test calls a cgo function. I am following the pattern we established but this whole file will be refactored. There is an issue for that.

geth/params/validator.go
func NewValidator()
Can we simplify by return validator.New() ?

👍 There are no custom validators, it can be simplified.

geth/common/types.go
APIDetailedResponse
APIFieldError
APIError
Why this API prefix ? Can we simplify to something idiomatic ?
Eg. Response, Field, Error

Also following convention here. In my opinion we shouldn't have something like common/types.go at all but each subpackage could have types.go file. Again, it will be easier to refactor if I am consistent now.

We are using single char to point to the struct, so please, stick with that.
Ex.
func (fe APIFieldError) Error()
We can just use "f" or "e".

👍 Yeah, I will change this.

[tiabc]

@MarinX, you're saying right things but not in the right time. We have a set of rules which should follow to avoid hell with merging: https://hackmd.io/s/rkgqzRWIb
@adambabik said absolutely right that it's easier to refactor if he now follows the pattern. If you have thoughts for improving code quality, you're welcome to add what's missing in #200

[MarinX]

Seems like a plan.
Approving it then.

[pedropombeiro]

@adambabik I noticed what seems a mismatch in the value for the warning level. It is mentioned as being WARNING in https://github.com/status-im/status-go/blob/develop/geth/params/config.go#L292, but WARN in https://github.com/status-im/status-go/blob/develop/cmd/statusd/main.go#L33. Which one is correct? I've contemplated both possible values for https://github.com/status-im/status-react/pull/2803/files#diff-f0c84ad5ab89ec75142eeb77847c830cR73, but would rather be sure.

[adambabik]

@pombeirp It should be WARN because of https://github.com/ethereum/go-ethereum/blob/master/log/logger.go#L69-L86.

[pedropombeiro]

@adambabik do you want me to change the code in https://github.com/status-im/status-go/blob/develop/geth/params/config.go#L292 to WARN? Otherwise there should be a mapping further up the call chain, in places like https://github.com/status-im/status-go/blob/develop/cmd/statusd/main.go#L103. I realize it's not an issue today because the WARN level probably isn't being used by anyone, but it would still be better to have these layers harmonized and working together.

[adambabik]

@pombeirp

do you want me to change the code in https://github.com/status-im/status-go/blob/develop/geth/params/config.go#L292 to WARN?

Yes, please. For now, that should be quite enough, however, I am not sure status-react validates config before starting a node.

[pedropombeiro]

@adambabik yeah, even if it doesn't, it will avoid inducing people new to the codebase in error. I initially used WARNING instead of WARN after looking at this code.