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

Updates for v4 installation steps #178

Closed
wants to merge 4 commits into from
Closed

Updates for v4 installation steps #178

wants to merge 4 commits into from

Conversation

cbrit
Copy link
Member

@cbrit cbrit commented Sep 9, 2022

Closes #124

@cbrit cbrit requested a review from EricBolten September 9, 2022 15:32
@cbrit cbrit temporarily deployed to CI September 9, 2022 15:39 Inactive
@cbrit cbrit temporarily deployed to CI September 9, 2022 15:39 Inactive
@cbrit cbrit temporarily deployed to CI September 9, 2022 15:53 Inactive
@cbrit cbrit temporarily deployed to CI September 9, 2022 15:53 Inactive
@cbrit cbrit temporarily deployed to CI September 9, 2022 21:25 Inactive
@cbrit cbrit temporarily deployed to CI September 9, 2022 21:25 Inactive
Copy link

@JimR8 JimR8 left a comment

Choose a reason for hiding this comment

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

New docs are looking good! I have not rebuilt a node using them but took a quick look over the commands and with the addition of the services files you have added the only other comments i could add is propose a change below to replace wildcard with binary name incase 'install' directory exists and contains other files

Original command: wget https://github.com/PeggyJV/steward/releases/download/v2.0.2/steward && chmod +x * && sudo mv * /usr/bin
Proposed command: wget https://github.com/PeggyJV/steward/releases/download/v2.0.2/steward && chmod +x steward && sudo mv steward /usr/bin

Also suggestion for your consideration that creating service account users as part of the process could be a good security improvement as well as allow the docs to contain correct paths when referencing users home directories for keystore and configs currently the process uses a user called ubuntu. This would require a lot of changes to the docs and files and if desirable maybe better added at a later stage so we can ship the changes and get the docs updated.

@cbrit cbrit mentioned this pull request Sep 12, 2022
@cbrit cbrit temporarily deployed to CI September 12, 2022 16:04 Inactive
@cbrit cbrit temporarily deployed to CI September 12, 2022 16:04 Inactive
Copy link
Contributor

@EricBolten EricBolten left a comment

Choose a reason for hiding this comment

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

A couple thoughts.


# Install Sommelier
wget https://github.com/PeggyJV/sommelier/releases/download/v3.1.1/sommelier_3.1.1_linux_amd64.tar.gz && tar -xf sommelier_3.1.1_linux_amd64.tar.gz && sudo mv sommelier /usr/bin && rm -rf sommelier_3.1.1_linux_amd64* LICENSE README.md
wget https://github.com/PeggyJV/sommelier/releases/download/v4.0.1/sommelier_4.0.1_linux_amd64.tar.gz && tar -xf sommelier_4.0.1_linux_amd64.tar.gz && sudo mv sommelier /usr/bin && rm -rf sommelier_4.0.1_linux_amd64* LICENSE README.md
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 nice that we provide direct commands, but every time the version of the node bumps (e.g. we just had a patch update for Dragonberry) we'd have to update these commands. Is there a way to point to "latest" or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a latest page but I don't see how you could select a specific binary architecture without being explicit about the file name. Maybe we just make this a manual step?

# - a key named "validator" with funds on the cosmos chain in the sommelier keystore

# Add the peers from contrib/mainnet/sommelier-3/peers.txt to the ~/.sommelier/config/config.toml file
nano ~/.sommelier/config/config.toml

# pull the genesis file
# pull the genesis file
Copy link
Contributor

Choose a reason for hiding this comment

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

Given there has been an upgrade in the intervening time, I don't think downloading the v4 binary will work for syncing from the genesis, which started on v3. Should we provide some instructions to use Cosmovisor or how to handle this manually?

$(steward --config $HOME/steward/config.toml keys eth show signer) \ # eth signer address
$(steward --config $HOME/steward/config.toml sign-delegate-keys signer $(sommelier keys show validator --bech val -a)) \
--chain-id sommelier-3 \
--from validator \
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC some folks were having issues with setting gas correctly for set-delegate-keys, do we need to specify it manually?

@@ -161,7 +182,7 @@ gorc eth-to-cosmos \
--erc20-address="0x0000000000000000000000000000000000000000" \
--amount="1.3530000" \
--cosmos-destination="$(sommelier keys show orchestrator -a)"

Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to remove these from the default README, there's really no need for an individual validator to bridge assets during their setup, and the extra traffic spends ETH.


[keys]
delegate_key = "orchestrator" # Edit if your orchestrator's Cosmos key name is different
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment indicating delegate_key is only used by steward and not the orchestrator?

@github-actions
Copy link

github-actions bot commented Dec 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Dec 4, 2022
@EricBolten EricBolten removed the Stale label Dec 5, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Mar 7, 2023
@github-actions github-actions bot closed this Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation update
3 participants