Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up gconf implementation #456

Closed
ethanfrey opened this issue Mar 29, 2019 · 3 comments · Fixed by #503
Closed

Clean up gconf implementation #456

ethanfrey opened this issue Mar 29, 2019 · 3 comments · Fixed by #503
Assignees

Comments

@ethanfrey
Copy link
Contributor

ethanfrey commented Mar 29, 2019

Is your feature request related to a problem? Please describe.
#294 brought in an initial (minimum) global configuration. This was good to unblock us an give us experience of what is needed, but several shortcomings have been determined.

The main issue is that there is no validation or normalization of the data when importing from the genesis file. We end up with text like: {"//amount":"0.1IOV","fractional":100000000,"ticker":"IOV"} or "0.01 IOV" stored under gconf:cash:minimal_fee. This has the problem of dumping all json parse logic on the client side, as well as delaying all validation until a first transaction that accesses the config, rather than on initialization time.

A second issue is no clean way to query for gconf client side (eg. register a bucket, path)

A third issue is that there is no way to update these values with transactions, or add new entries besides genesis. The third issue will be a separate point, cleaning up querying is the main point of this issue.

Describe the solution you'd like

  • Use a bucket to store gconf value and register it for queries (for consistency)
  • Define expected types for all gconf variables to be present in genesis file, and validate the input. We can have a "required" flag, which will return error if not-present, otherwise it can use the zero value if not present (eg. no minimum fee, fees go to burn address...)
  • Store all gconf values in a normalized (binary) format in the KVStore. coin.Coin{} protobuf encoded, int64 as protobuf or simple bigendian encoding, etc...

The end result is the client can do something like (pseudocode)

var minFee coin.Coin
data := client.query("/gconf", "cash:minimal_fee")`;
data.Unmarshal(&minFee)

And not have to reinvent the json parsing logic (which is getting complex, especially for addresses) on the client side. We should do that server side when importing.

Describe alternatives you've considered
Throwing in raw json into the store... not so pretty.

Update

Going with approach from #503 and #534
Gconf is a library with InitConfig LoadConfig GConfHandler() etc that can be used by various packages. Each module maintains it's own custom Configuration struct and we use strong typing.

@husio husio self-assigned this Apr 4, 2019
husio added a commit that referenced this issue Apr 4, 2019
@husio husio mentioned this issue Apr 4, 2019
6 tasks
@husio
Copy link
Contributor

husio commented Apr 5, 2019

There is an initial implementation but finishing it with current assumptions is not possible #491 (comment)

This approach requires adjusting as JSON data does not provide a schema.

@husio husio removed their assignment Apr 5, 2019
@husio
Copy link
Contributor

husio commented Apr 8, 2019

Currently we are using gconf package for only two variables, both used only by the x/cash extension

cash.GconfCollectorAddress: "cond:dist/revenue/0000000000000001",
cash.GconfMinimalFee: "0.01 FRNK",

All gconf variables are required. Program will panic if the variable does not exist or is of a wrong type.
Why not instead of using gconf explicitely pass those variables during the handler initialization? I think that would be a perfect solution, but it has two issues:

  • setting a value requires code change and we want to use genesis
  • we want those variables to be updated during runtime. For example a minimal fee is updated from 1 IOV to 3 IOV

We want to use typed configuration, which is very much like a model. Model already supports loading from genesis and it is stored with a type information.

After my latest attempt and considering that only one extension is using gconf I think we should remove gconf package. Instead of using gconf, each extension should define a configuration model together with genesis initializer.

  • ➕ each extension controls the type of each configuration
  • ➕ genesis loader can provide default values for each configuration variable
  • ➕ genesis loader can fail if a configuration value is missing or is not acceptable
  • ➕ if needed a message to update configuration can be implemented
  • ➖ each extension must write similar (not the same!) code, similar to other models
  • ➖ there is only one instance of a configuration so creating a bucket for it makes little sense
  • ➖ each configuration is stored in a separate namespace and therefore cannot be queried as requested (data := client.query("/gconf", "cash:minimal_fee"))

@husio husio added question blocked Work on the issue cannot be continue before another problem is solved. and removed question blocked Work on the issue cannot be continue before another problem is solved. labels Apr 8, 2019
husio added a commit that referenced this issue Apr 8, 2019
Cleanup `gconf` package configuration.

Instead of allowing for any key-value pairs inserted into gconf store,
require each module to use a custom protobuf configuration object that
will be stored and retrieved with the help of `gconf` package.

resolve #456
@husio husio mentioned this issue Apr 8, 2019
@husio husio self-assigned this Apr 8, 2019
@ethanfrey
Copy link
Contributor Author

That approach seems fine with me

each configuration is stored in a separate namespace and therefore cannot be queried as requested (data := client.query("/gconf", "cash:minimal_fee"))

Not really a problem. If we query eg. ("/cashconf", "minimal_fee") that is fine.

Just some path without raw / query

@husio husio mentioned this issue Apr 11, 2019
@husio husio removed their assignment Apr 11, 2019
@husio husio self-assigned this Apr 18, 2019
husio added a commit that referenced this issue Apr 18, 2019
Cleanup `gconf` package configuration.

Instead of allowing for any key-value pairs inserted into gconf store,
require each module to use a custom protobuf configuration object that
will be stored and retrieved with the help of `gconf` package.

resolve #456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants