Cosmos-sdk: Remove dependencies on the staking module in `gentx` and `collect-gentxs` commands

Created on 7 Sep 2020  ·  10Comments  ·  Source: cosmos/cosmos-sdk

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 :man_astronaut:

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

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


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned

Most helpful comment

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

All 10 comments

No need for both #4771 and this issue.

@ethanfrey @aaronc

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

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

    1. 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?

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

Sorry, what is the exact proposal here?

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

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.

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

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!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

rigelrozanski picture rigelrozanski  ·  3Comments

rigelrozanski picture rigelrozanski  ·  3Comments

mossid picture mossid  ·  3Comments

rigelrozanski picture rigelrozanski  ·  3Comments

faboweb picture faboweb  ·  3Comments