-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Comments
No need for both #4771 and this issue. |
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 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 But was removed from My main thoughts on why this is the correct approach are:
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 Thanks for your input @ethanfrey ❤️ @alexanderbez basically just:
Can be seen here in |
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! |
closing as users are recommended to write their own, and Robert merged a pr to help with this |
Summary
REF #4422
REF #4771
When using the
gentx
andcollect-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 thegenutil
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:launchpad
andstargate
we need updates:launchpad
=> extend the interface defined in the =>gentx.go
stargate
=> rebuild the interface => ?ValidateAccountInGenesis
function to the interfacecollect-gentx
With regards to the
collect-gentx
command:/client/*d.go
fileI'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
The text was updated successfully, but these errors were encountered: