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

Secrets manager introduced to commands #1251

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

Nemanja0x
Copy link
Contributor

Description

Commands stake, unstake, withdraw, validator info use secret manager to provide access to cloud secret storage or local disc.
Naming of the flags are standardized for all commands:
DataPathFlag = "data-dir" - for local storage
ConfigFlag = "config" - cloud config

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

@Nemanja0x Nemanja0x marked this pull request as ready for review February 25, 2023 15:47
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally looks good.

Points to consider addressing:

  • What should we do with command/rootchain/emit/emit.go and command/rootchain/initcontracts/init_contracts.go? Those two commands are expecting hex-encoded private key to be provided via the CLI flag, but not sure if is it ok to leave it like that.
  • Also just mark DataPathFlag and ConfigFlag mutually exclusive in each command it accommodates it.

command/polybftsecrets/utils.go Show resolved Hide resolved
command/sidechain/withdraw/withdraw.go Show resolved Hide resolved
command/sidechain/helper.go Show resolved Hide resolved
command/sidechain/helper.go Outdated Show resolved Hide resolved
command/sidechain/helper.go Show resolved Hide resolved
command/polybftsecrets/utils.go Show resolved Hide resolved
@Nemanja0x Nemanja0x merged commit d815a3f into develop Feb 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2023
@vcastellm vcastellm deleted the EVM-488-commands-use-secret-manager branch February 27, 2023 14:34
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Late review but shared flags between subcommands can be defined in the top level command if I'm not wrong.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants