search

Onyx-Protocol / Onyx

Onyx
https://Onyx.org
GNU Affero General Public License v3.0
1.79k stars 362 forks source link

core: add enclave config option #1430

Closed jbowens closed 7 years ago

jbowens commented 7 years ago

Allow configuring Enclave URLs and access tokens through a new corectl configuration option.

jbowens commented 7 years ago

Left a couple o Q's as review comments.

Also wondering:

kr commented 7 years ago

Should the option be called enclaves? enclave_urls? block_enclaves? signer_enclave_urls?

I have some idiosyncratic preferences here (so maybe take it with a grain of salt). I would lean toward singular, so it reads better in the add command (as opposed to the get command, where if you like you can read it as if the word "list" is implied at the end "get enclaveurl list"). And I think underscores (and hyphens) are ugly, so I would go for enclave or enclaveurl.

corectl add enclave http://foo/ xyz
or
corectl add enclaveurl http://foo/ xyz

I think we should prob not put "block" or "signer" in the name because there's no reason cored couldn't use this config for other enclave functions if necessary in the future. It's really just a set of fungible local Chain Enclave instances to talk to.

Should config.Options.DefineSet return a type that includes the key name so that other code doesn't need to hard-code "enclaves"?

Ahh interesting. I can think of two reasons that would help. One is if we want to rename a config var, which we should really just not do, and the other is if we have typos in the name that won't be caught until runtime. But there won't be many places these names appear (once where it's defined, and once where we grab the live-updating accessor function) so I feel like the chance of this being a problem is low. WDYT?

On main, the presence of a HSM URL is used to determine whether MockHSM or EnclaveClient is used. Is it ok to always use MockHSM if Chain Core is compiled with MockHSM, and always use EnclaveClient if it's not?

Great idea. We should totally do that.

jbowens commented 7 years ago

Any thoughts on where to put the migration code? As we add additional config options, we might accumulate quite a bit of migration code.

kr commented 7 years ago

Hmm as long as the migration code is confined to either the config package or a function in chain/core with "config" in the name (I think that's where it is now) I am not too worried if there's a lot of it, at least it's nicely contained and won't clutter up the rest of the codebase.

kr commented 7 years ago

LGTM after one possible small thing to add