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

Align bootstrapping docs and CLI commands with the latest updates. #145

Merged
merged 15 commits into from
Feb 9, 2024

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Feb 8, 2024

Description

  • Use in-memory keyring for the relayer
  • Improve the logging of the processes
  • Rename processes variables

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@dzmitryhil dzmitryhil changed the title Update bootstrapping docs and CLI commands with the latest updates Align bootstrapping docs and CLI commands with the latest updates. Feb 8, 2024
@dzmitryhil dzmitryhil marked this pull request as ready for review February 8, 2024 13:23
@dzmitryhil dzmitryhil requested a review from a team as a code owner February 8, 2024 13:23
@dzmitryhil dzmitryhil requested review from miladz68, ysv and wojtek-coreum and removed request for a team February 8, 2024 13:23
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r1, all commit messages.
Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


README.md line 40 at r1 (raw file):

```bash
alias coreumbridge-xrpl-relayer="docker run -it --rm -v $HOME/.coreumbridge-xrpl-relayer:/root/.coreumbridge-xrpl-relayer coreumfoundation/coreumbridge-xrpl-relayer:$RELEASE_VERSION"

does it work in zshell?


README.md line 40 at r1 (raw file):

```bash
alias coreumbridge-xrpl-relayer="docker run -it --rm -v $HOME/.coreumbridge-xrpl-relayer:/root/.coreumbridge-xrpl-relayer coreumfoundation/coreumbridge-xrpl-relayer:$RELEASE_VERSION"

set here an option to run it with corret uid and gid. Otherwise all the generated files ill be owned by root on the host machine.


README.md line 49 at r1 (raw file):

    --coreum-chain-id $COREUM_CHAIN_ID \
    --coreum-grpc-url $COREUM_GRPC_URL  \
    --xrpl-rpc-url $XRPL_RPC_URL   

what about metric server address? Default one is "localhost:9090", to make it accessible from host os you need to set it to ":9090", and for macs you need to additionally map port (-p) in the docker command


relayer/logger/yaml_console.go line 450 at r1 (raw file):

func (c *yamlConsoleEncoder) appendCustomTypes(value interface{}) bool {
	if addr, ok := value.(sdk.AccAddress); ok {

switchwould be better here

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

* Simplify appendCustomTypes for the yaml logger
Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


README.md line 40 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

set here an option to run it with corret uid and gid. Otherwise all the generated files ill be owned by root on the host machine.

Done


README.md line 40 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

does it work in zshell?

I don't have the zshell to test.


README.md line 49 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

what about metric server address? Default one is "localhost:9090", to make it accessible from host os you need to set it to ":9090", and for macs you need to additionally map port (-p) in the docker command

By default the metrics are disabled so no need to provide the host.

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


relayer/logger/yaml_console.go line 450 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

switchwould be better here

Done.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


README.md line 40 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I don't have the zshell to test.

brew install zsh

relayer/logger/yaml_console.go line 453 at r3 (raw file):

	case sdk.AccAddress:
		c.AppendString(v.String())
	case sdkmath.Int:

no need to handle sdk.Int separately?

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


README.md line 40 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…
brew install zsh

works


relayer/logger/yaml_console.go line 453 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

no need to handle sdk.Int separately?

Nope, sdkmath.Int is sdk.Int

wojtek-coreum
wojtek-coreum previously approved these changes Feb 9, 2024
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil and @miladz68)


README.md line 34 at r3 (raw file):

Install relayer (with docker)

Shall we add Prerequisites section ? I guess only docker should be installed ?


README.md line 36 at r3 (raw file):

### Install relayer (with docker)

For the document simplicity we use the alias for the command which will be executed in docker.

is it the doc which we will share with our relayers ?
I think for now it looks a bit unclear where to start and where to stop.
Maybe we should add a separate doc for them with all the steps thy are supposed to do ?

or lets discuss


README.md line 61 at r3 (raw file):

```bash
coreumbridge-xrpl-relayer keys-coreum add coreum-relayer

nit: I that most of our relayers are likely to already have generated mnemonics
Is --recover option supported here ? Shall we mention that they could use existing mnemonic ?

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


README.md line 34 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Shall we add Prerequisites section ? I guess only docker should be installed ?

But that's obvios IMO. To have it just for docker is too much.


README.md line 36 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

is it the doc which we will share with our relayers ?
I think for now it looks a bit unclear where to start and where to stop.
Maybe we should add a separate doc for them with all the steps thy are supposed to do ?

or lets discuss

We will re-write it and move to docs, the dev-rel team will help with it.


README.md line 61 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: I that most of our relayers are likely to already have generated mnemonics
Is --recover option supported here ? Shall we mention that they could use existing mnemonic ?

This doc desribes the simplies option. But --recover make sense. Updated.

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @wojtek-coreum)


README.md line 34 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But that's obvios IMO. To have it just for docker is too much.

if this is only docker then ok
just devrel in their doc should probably mention it

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 10 files at r1, 1 of 2 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

@dzmitryhil dzmitryhil removed the request for review from wojtek-coreum February 9, 2024 14:59
@dzmitryhil dzmitryhil merged commit e6b8036 into master Feb 9, 2024
7 checks passed
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