[KEYBASE] Refactor keybase config protos to be more modular

0xBigBoss commented 1 year ago

Objective

Current, keybase config proto relies on an overloaded, generic struct to configure connecting to a keybase. A more modular approach could use the proto oneof operator.

E.g.

message KeybaseConfig {
    oneof keybase_config {
        KeybaseFileConfig file_config = 1;
        KeybaseVaultConfig vault_config = 2;
    }
}

message KeybaseFileConfig {
    string file_path = 1;
}

message KeybaseVaultConfig {
      string vault_addr = 1;
      string vault_token = 2;
      string vault_mount_path = 3;
}

Origin Document

_Originally posted by @Olshansk in https://github.com/pokt-network/pocket/pull/537#discussion_r1145620918_

Goals

Simplifies the keybase config
Allows for easier configuration of new keybase types in the future

Deliverable

[ ] Refactor the keybase_config.proto to be more modular
[ ] Update the CLI to parse and read config.json using the new keybase config protos
[ ] Update default configuration to use the new module KeybaseFileConfig.

Non-goals / Non-deliverables

Integrating keybase logic into node runtime (unless this functionality already exists on main)

General issue deliverables

[ ] Update the appropriate CHANGELOG(s)
[ ] Update any relevant local/global README(s)
[ ] Update relevant source code tree explanations
[ ] Add or update any relevant or supporting mermaid diagrams

Testing Methodology

[ ] Task specific tests or benchmarks: make ...
[ ] New tests or benchmarks: make ...
[ ] All tests: make test_all
[ ] LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @0xBigBoss

0xBigBoss commented 1 year ago

I played around with the new config @Olshansk and hit a hiccup with Viper. As I suspected Viper is having trouble unmarshalling from JSON. Essentially we would have to now tell Viper about all possible options since the default config does not have any vault fields. Even then, I'm not sure how this would work exactly without being explicit and parsing the keybase config explicitly.

You can see in this screenshot when specifying Vault in the config.json Viper defaults to file. And ultimately, chooses the keybase file config (instead of the correct vault).

https://github.com/pokt-network/pocket/compare/0xbigboss/fix-626-modularize-keybase-config

Not sure this PR is worth it since the config parsing logic is still sort-of simple. Would hate to introduce a bunch of complexity to handle this modularization.

Olshansk commented 1 year ago

@0xBigBoss I don't have a good answer though I would prefer to modularize the configs if possible.

Thoughts on cross-posting this issue (with the example you have) to the viper repo and see if their devs have any good suggestions?

Olshansk commented 1 year ago

Saw your comment in #656.

I think we should get some support from the viper team and maybe just add a comment in the code for now linking to this ticket showing IMPROVE(#626): we have proposed a better config design but did not implement it due to viper limietations. Could even link directly to the viper issue we open up directly.

0xBigBoss commented 1 year ago

That sounds good @Olshansk . FWIW I think it would be better posted to the https://github.com/mitchellh/mapstructure repo since that's what Viper is using to do the decoding.

I'll see if I can open an issue there summarizing the issue. I think it comes down to a more complicated decoder function, but will see if they have a trick or two to handle it more easily.

0xBigBoss commented 1 year ago

Spent a little reading on this and found that Viper will point to the mapstructure repo on any decoding questions/issues. I did open a discussion on Vipers repo https://github.com/spf13/viper/discussions/1534.

Reading thru the mapstructure repo finds an open issue without any support on how a developer solved a similar issue by implementing their own decoding logic. https://github.com/mitchellh/mapstructure/issues/293 Also, it seems mapstructure is not really maintained anymore.

Which leaves this feature almost there in terms of we have the custom decoder function working, but the decoder does not handle the various keybase configurations that can be provided and needs more logic to determine the users "intent."

I still believe the best approach here is to keep the keybase enum type FILE or VAULT and that determines which keybase is decoded so it is unambiguous.

Olshansk commented 1 year ago

Which leaves this feature almost there in terms of we have the custom decoder function working, but the decoder does not handle the various keybase configurations that can be provided and needs more logic to determine the users "intent."

If we copy-paste some of the logic in mapstructure, would this be possible then or does it make things too complicated?

If it's a reasonable amount of work we can port over - seems reasonable to do it.
If it's not - we can just close this out and leave a comment in the code saying "See #626 for rationale around proto design"

pokt-network / pocket