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

Remove dependencies on the staking module in gentx and collect-gentxs commands #7253

Closed
4 tasks
PaddyMc opened this issue Sep 7, 2020 · 11 comments
Closed
4 tasks
Assignees

Comments

@PaddyMc
Copy link
Contributor

PaddyMc commented Sep 7, 2020

Summary

REF #4422
REF #4771

When using the gentx and collect-gentx commands, the cosmos-sdk has hard-coded dependencies on the staking module, this makes it difficult to use any other consensus algorithms while also trying to leverage the genutil module.

I came upon this issue while building a Proof Of Authority consensus module for the cosmos-sdk.

Proposal

gentx

With regards to the gentx command:

  1. For both launchpad and stargate we need updates:
    1. launchpad => extend the interface defined in the => gentx.go
    2. stargate => rebuild the interface => ?
  2. Add the ValidateAccountInGenesis function to the interface
  3. Let the developer satisfy the interface as seen in gaiad

collect-gentx

With regards to the collect-gentx command:

  1. Abstract the msg validation to the staking module
  2. Pass the msg validation function into the cmd in the /client/*d.go file
  3. Let the function be passed down to where it's eventually used

I'm happy to build the feature mentioned above if everyone thinks it's the correct way to tackle the problem 👨‍🚀

If anyone has any suggestions on the issue or on how to handle the implementation would love to discuss ❤️

cc @marbar3778 @okwme @anyoneelsewhowantstoinput @:+1:


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

No need for both #4771 and this issue.

@alessio
Copy link
Contributor

alessio commented Sep 8, 2020

@ethanfrey @aaronc

@ethanfrey
Copy link
Contributor

gentx is tied to the staking module.

I would simply disable the command in the client when using the poa consensus model. Taking gaia for an example, remove the following lines:

https://github.com/cosmos/gaia/blob/main/cmd/gaiad/main.go#L54-L55
https://github.com/cosmos/gaia/blob/main/cmd/gaiad/main.go#L52

If you need to add other poa-specific commands, add them there.

No need to over-abstract some magic gentx cli tools. Let's just make each command work for one usecase and optionally enable/disable them. It is easy enough to write your own cobra command

@PaddyMc
Copy link
Contributor Author

PaddyMc commented Sep 8, 2020

Hmm, that's an interesting approach, normally I'd completely agree with the complete separation of concern but I feel in this instance it's actually easier to abstract the logic and pass in the 5 functions that will be needed.


WRT having a staking specific command:

This functionality was nearly fully-implemented in the launchpad (only missing one function):

But was removed from stargate:


My main thoughts on why this is the correct approach are:

  1. Every chain will be able to be initialized the same way (which is well documented)

    1. init
    2. add key
    3. add-genesis-account
    4. gentx
    5. collect-gentx
  2. Remove the need to copy/pasta two commands from the sdk into custom applications and edit them to suit to needs of custom consensus algorithms.

  3. Leverage code that's already written

  4. Will enforce cosmos devs to use genesis transactions as opposed to hardcoding values in the genesis.json and have complicated logic in the InitGenesis function of the consensus module


Maybe I'm misunderstanding here, just my two cents 🙇

Thanks for sharing your thoughts here @ethanfrey they're greatly appreciated 🤝

wdyt?

@ethanfrey
Copy link
Contributor

My thought is you may be perfectly correct to update stargate like this.

We are keeping launchpad very conservative for changes. If it is not a bug, but rather a feature request (it is) and it modifies some public APIs (it does, right?), then it does not make it into a SRU.

As to the stargate/master changes, if @alexanderbez and @alessio think this is a good design, I support them

@alexanderbez
Copy link
Contributor

Sorry, what is the exact proposal here?

@PaddyMc
Copy link
Contributor Author

PaddyMc commented Sep 8, 2020

Completely understand and completely agree, this should not be pushed this into launchpad if there are any breaking changes.

Thanks for your input @ethanfrey ❤️

@alexanderbez basically just:

  1. Removing all references to the staking module from the genutil module
  2. Pass functions into the genutil module by implementing an interface

Can be seen here in launchpad
https://github.com/cosmos/cosmos-sdk/blob/v0.39.1/x/genutil/client/cli/gentx.go#L35-L40

@alexanderbez
Copy link
Contributor

ACK to the proposal 👍 we can slate this for 0.41. Feel free to open a PR though @PaddyMc and maybe we can get it into stargate.

@PaddyMc
Copy link
Contributor Author

PaddyMc commented Sep 9, 2020

Sounds good, I'll create a PR and we can go from there 🤝

@alessio
Copy link
Contributor

alessio commented Sep 9, 2020

I'm thrilled, thus ACK to the proposal from me too. And once it is merged into Stargate I'd love to see it merged into Launchpad too!

@tac0turtle
Copy link
Member

closing as users are recommended to write their own, and Robert merged a pr to help with this

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 a pull request may close this issue.

5 participants