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

NTRN-195 feat: support all possible keyrings #53

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

oldremez
Copy link

@oldremez oldremez commented Oct 31, 2022

This PR contains changes intended to add support for all possible keyrings to the query-relayer.

Changes:

  1. It's now possible to use any keyring supported by keyring lib.
  2. Memory keyring is supported in a way where user can provide mnemonic via env variable.
    1. in such case, there is no need to require the keyname to be passed from the user
    2. it's also a bad idea in my opinion to use any default value for keyname option since it might affect existing accounts in case of the wrong configuration
    3. that's why keyname can be modified during execution and all users of the keyring should take not the one from config but the one from keyring initialization
  3. kwallet hasn't been tested properly since it's some old-ass almost-deprecated UI-with-KDE-only thing that even has no tests in the original go library. Not sure if somebody in Cosmos even uses it, I'd assume not. Even if yes, it's a bad idea to use it in query-relayer deployment. But it should hopefully work anyway.
  4. Config modified in a way where it has a group of dependent parameters. Those depend on keyring_backend type mostly and it's covered in docs icq relayer keyring configuration added neutron-docs#35
  5. The hack of substitution of the KeyDirectory in the CosmosProvider before initialization is replaced with the hack of substitution of the Keyring itself after the initialization. It's done for multiple reasons:
    1. The abstraction is broken here. CosmosProvider should accept arbitrary keyring instead of making it by itself. That's why I see the hack with the substitution of the whole keyring to be a more proper approach.
    2. The way CosmosProvider creates Keyring is also not flexible (the necessity of KeyDirectory is the first proof; the second one is an absence of the ability to provide HDPath when creating the key in the implicitly created Keyring)
  6. In the relay/config.go both chains are created with empty memory Keyrings instead of "test"/configured one. The reasons are:
    1. Memory is a better placeholder for Keyring than "test" since it just creates in-memory empty object without any link to the outside environment
    2. The Neutron CosmosProvider is created with an empty Keyring explicitly because we're going to replace it further. Passing "real" values would increase the confusion and probability of errors in my opinion.
  7. The appName for Keyring creation is taken from the neutronapp.Bech32MainPrefix which is wrong and should be fixed but there is no proper way to get appName so far since AFAIK it is defined in neutron's Makefile only
  8. In the documentation I added the recommendation to use memory backend or file. file/pass are recommended in the Cosmos docs as preferable in headless environments, memory is also safe since mnemonic is passed via env. test is insecure, os requires to provide os-level secret to be provided to relayer or something, and for kwallet see point 3 above.
  9. Integration tests are here NTRN-195 feat: relayer keyring tests neutron-integration-tests#31
  10. I had to add run_dev.sh script to replace beautiful export $(grep -v '^#' .env | xargs) && make dev command. The reason is there is no way to export env variables containing spaces (e.g. seed phase) in a one-liner (at least I tried and failed).

@oldremez oldremez marked this pull request as ready for review November 16, 2022 13:35
Copy link
Collaborator

@pr0n00gler pr0n00gler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, run goimports -w -local github.com/neutron-org .

README.md Outdated Show resolved Hide resolved
internal/relayer_keyring/relayer_keyring.go Outdated Show resolved Hide resolved
internal/app/app.go Outdated Show resolved Hide resolved
internal/app/app.go Outdated Show resolved Hide resolved
@@ -31,20 +32,24 @@ type NeutronQueryRelayerConfig struct {

const EnvPrefix string = "RELAYER"

// NeutronChainConfig TODO: research if HomeDir parameter is needed for something but keys (so it might be optional and ignored for memory keyring)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's passed to the GetNeutronChain besides its keyring's usage. you can also inspect your ~/.neutrond directory which is the default value for neutrond cli calls and probably come to a conclusion about this TODO:

neutrond --help

...
      --home string         directory for config and data (default "~/.neutrond")
...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I mean of course it's passed there but is it actually needed there?

internal/relay/config.go Show resolved Hide resolved
internal/relay/config.go Outdated Show resolved Hide resolved
internal/relayer_keyring/relayer_keyring.go Outdated Show resolved Hide resolved
RESTAddr string `required:"true" split_words:"true"`
HomeDir string `required:"true" split_words:"true"`
SignKeyName string `split_words:"true"`
SignKeySeed string `split_words:"true"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config is getting bigger and bigger, how do you feel to move keyring related fields to a standalone struct, or at least place the similar fields next to each other.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, makes sense, did it

@oldremez oldremez changed the title feat: support all possible keyrings NTRN-195 feat: support all possible keyrings Dec 9, 2022
# Conflicts:
#	internal/app/app.go
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 this pull request may close these issues.

4 participants