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

Add bond denom flag during gaiad init #5228

Closed
4 tasks
kwunyeung opened this issue Oct 21, 2019 · 24 comments
Closed
4 tasks

Add bond denom flag during gaiad init #5228

kwunyeung opened this issue Oct 21, 2019 · 24 comments

Comments

@kwunyeung
Copy link
Contributor

Summary

Currently the default bond_denom is stake in during gaiad init. If the bond denom is not stake for a chain, we have to manually change the bond_denom in the generated genesis file. I suggest to add a --bond-denom flag in the init command to define the bond denom when the genesis file is first generated.

Problem Definition

It's easier to define the bond denom before adding genesis accounts and gentx. And it's one more step less for setting a correct genesis file.

Proposal

add a --bond-denom flag in init command of genutil module.


For Admin Use

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

alexanderbez commented Oct 21, 2019

Thanks for raising concern @kwunyeung and @RiccardoM. I'll mention my points from Telegram. I don't see how this will prove to be valuable and here is why:

  1. There's over 20+ params from the core SDK modules (all documented btw 🎉). What makes BondDenom special? Where do you draw the line for what params to support in the init command? How does this work if we got equivalent requests for 10 different params?
  2. This only pertains to rapid prototyping and local/dev networks, not main/production networks. That being said, you can quickly execute a python on bash script to modify all and any params you want. Different applications will require editing different params.

I get (2) is somewhat subjective, but before proceeding, let's discuss (1) at least :)

@kwunyeung
Copy link
Contributor Author

Yes there are over 20 params from the SDK but I would say the BondDenom is the one that developers would have to change every time a new chain is going to be developed. This is like how the SDK can be configured with coinType and bech32Prefix with their own requirements. And they just have to this once. If not in the init command, I would prefer it should be done similar to the SDK config.

The other params are important too but I feel that many developers will just stick with the params what Cosmos Hub is running with when they first start. Making changes to the params like slashing or minting in the genesis file is actually more handy as dev can do experiments with the changes of the params by just editing the genesis file and restart.

@haasted
Copy link
Contributor

haasted commented Oct 22, 2019

If it is primarily meant to be used when developing new chains, it might be worth considering just setting a new default genesis state for the staking module with the desired denomination. That would lock down the staking token for the chain once and for all.

@alexanderbez
Copy link
Contributor

I see. I still don't think this is valuable being that you can also argue that you should just stick with the default bond denom on local networks. Also, the Cosmos Hub ≠ SDK and we want to encourage developers and clients to be very aware and comfortable with the given params.

That being said, I'd be happy to review and accept a PR on this if it'll help you out this much 👍

@tuckermint
Copy link

Changing bond_denom in genesis.json isn't even working for me.
See cosmos/sdk-tutorials#168

@alexanderbez
Copy link
Contributor

Changing bond_denom in genesis.json isn't even working for me.
See cosmos/sdk-application-tutorial#168

Probably an issue with the tutorial app?

@RiccardoM
Copy link
Contributor

@alexanderbez @kwunyeung I personally like more the approach of implementing a new NewAppModuleBasic method into the staking module like I did in PR #5247. This would easily allow zones to customize the default genesis parameters without having to "elevate" the bond denom param above other parameters.

@alexanderbez
Copy link
Contributor

@RiccardoM thanks! Take a look at the feedback I left. It's not clear to me how that truly solves the grievances laid out here when rapid deploying dev/testnets.

@alessio
Copy link
Contributor

alessio commented Nov 29, 2019

We received a similar issue in the past: #4531.
I agree with @alexanderbez here, I think there is more value in making developers fully aware of all the parameters as it would encourage overall good practices.

@njmurarka
Copy link
Contributor

I will throw in my 2 cents here -- we are trying to launch a chain with our own token and it seems that the name of the token is important enough that this might be a good option to have when we run the init command. In fact, there might be others that are also good to add. I am still learning Cosmos so I can only comment on what I know so far.

@jackzampolin
Copy link
Member

I would support adding an optional flag for this! Think it's a nice UX affordance.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2020

This issue 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 Jul 5, 2020
@slider2007
Copy link

It was very nice and interesting discussion but i don’t see any positive useful outcome out of it. Changing this particular parameter only in genesis.json by hand is not enough that is for sure.

@github-actions github-actions bot removed the stale label Jul 8, 2020
@alexanderbez
Copy link
Contributor

PRs are welcome :)

@fer-correa
Copy link

I'll try to pick this issue...

@alexanderbez
Copy link
Contributor

I'm still against this proposal. There is nothing special about the default bond denom. You could make the argument about almost any default module's genesis parameter(s).

Plus, this can already be easily solved by setting the variable in your app. e.g.:

https://github.com/umee-network/umee/blob/1602136fd7524897c6dff9625e0ddeacba42c104/app/modules.go

@ismyhc
Copy link

ismyhc commented Jan 11, 2022

I'm still against this proposal. There is nothing special about the default bond denom. You could make the argument about almost any default module's genesis parameter(s).

Plus, this can already be easily solved by setting the variable in your app. e.g.:

https://github.com/umee-network/umee/blob/1602136fd7524897c6dff9625e0ddeacba42c104/app/modules.go

Hi @alexanderbez following along on this proposal, I was implementing your solution to setting custom bond denom.

My goal is to have a single token denomination that handles all staking/gov etc. Ive achieved this by following your solution, but there's one thing Im not quite sure about and its how the gov module is instantiated in the ModuleBasics.

Here's what my ModulesBasics looks like

	ModuleBasics = module.NewBasicManager(
		auth.AppModuleBasic{},
		genutil.AppModuleBasic{},
		bank.AppModuleBasic{},
		capability.AppModuleBasic{},
		stakingModule{}, //staking.AppModuleBasic{},
		mintModule{}, //mint.AppModuleBasic{},
		distr.AppModuleBasic{},
		govModule{}, //gov.NewAppModuleBasic(getGovProposalHandlers()...), < This is what Im unsure of?
		params.AppModuleBasic{},
		crisisModule{}, //crisis.AppModuleBasic{},
		slashing.AppModuleBasic{},
		feegrantmodule.AppModuleBasic{},
		ibc.AppModuleBasic{},
		upgrade.AppModuleBasic{},
		evidence.AppModuleBasic{},
		transfer.AppModuleBasic{},
		vesting.AppModuleBasic{},
		glxchainmodule.AppModuleBasic{},
		// this line is used by starport scaffolding # stargate/app/moduleBasic
	)

And here's what my govModule looks like

type govModule struct {
	gov.AppModuleBasic
}

func (govModule) DefaultGenesis(cdc codec.JSONCodec) json.RawMessage {

	genesis := govtypes.DefaultGenesisState()
	genesis.DepositParams = govtypes.DepositParams{
		MinDeposit:       sdk.NewCoins(sdk.NewCoin("glx", sdk.NewInt(10000000))),
		MaxDepositPeriod: DefaultPeriod,
	}

	return cdc.MustMarshalJSON(genesis)
}

How would handle passing in the proposal handlers to my overridden govModule? I'm unable to access the AppModuleBasic fields proposals field as its unexported?

Any help much appreciated.

Thanks!
Jacob

@alexanderbez
Copy link
Contributor

@ismyhc
Copy link

ismyhc commented Jan 11, 2022

Yes, good question. You can see how it's done here:

Perfect. Thank you!

@ismyhc
Copy link

ismyhc commented Jan 11, 2022

@alexanderbez sorry to bug you again, but Im having a hard time wrapping my head around a couple of things and also if this is the wrong place, please let me know where I can discuss this.

I've been looking at this: https://docs.cosmos.network/master/architecture/adr-024-coin-metadata.html

Which seems too still only be a proposal?

What I'm trying to achieve is one single token with 6 decimal places that's used for gov/staking, etc. Im failing to see how this is possible so far. Do you have a recommendation of how I could achieve this?

I've tried implemented my local chain the same as your examples here, but it seems very cumbersome. First after launching the chain, it seems that the user would actually need a balance of each type depending on whether they wanted to deal in fractions of the token. Second, even when using the cli, it just rounds down the amount I pass and only sends whole amounts. Forgive me for not understanding this, very new to cosmos. Coming from the EOS world.

metadata := banktypes.Metadata{
	Description: "The native staking token of the Galaxoid network.",
	Base:        BondDenom,
	Name:        "Galaxoid Coin",
	Display:     "GLX",
	Symbol:      "GLX",
	DenomUnits: []*banktypes.DenomUnit{
		{
			Denom:    BondDenom,
			Exponent: 0,
			Aliases: []string{
				"microglx",
			},
		},
		{
			Denom:    "GLX",
			Exponent: 6,
			Aliases:  []string{},
		},
	},
}

Are these 2 separate tokens? It seems that they are which to me is very confusing.

@alessio
Copy link
Contributor

alessio commented Jan 11, 2022

I think there is more value in making developers fully aware of all the parameters as it would encourage overall good practices.

Any comment on this? Developers must be fully aware of what they do. If they ain't, then it does not even resemble a problem to us.

Kindest Regards.

AT

@ismyhc
Copy link

ismyhc commented Jan 12, 2022

@alexanderbez sorry to bug you again, but Im having a hard time wrapping my head around a couple of things and also if this is the wrong place, please let me know where I can discuss this.

I've been looking at this: https://docs.cosmos.network/master/architecture/adr-024-coin-metadata.html

Which seems too still only be a proposal?

What I'm trying to achieve is one single token with 6 decimal places that's used for gov/staking, etc. Im failing to see how this is possible so far. Do you have a recommendation of how I could achieve this?

I've tried implemented my local chain the same as your examples here, but it seems very cumbersome. First after launching the chain, it seems that the user would actually need a balance of each type depending on whether they wanted to deal in fractions of the token. Second, even when using the cli, it just rounds down the amount I pass and only sends whole amounts. Forgive me for not understanding this, very new to cosmos. Coming from the EOS world.

metadata := banktypes.Metadata{
	Description: "The native staking token of the Galaxoid network.",
	Base:        BondDenom,
	Name:        "Galaxoid Coin",
	Display:     "GLX",
	Symbol:      "GLX",
	DenomUnits: []*banktypes.DenomUnit{
		{
			Denom:    BondDenom,
			Exponent: 0,
			Aliases: []string{
				"microglx",
			},
		},
		{
			Denom:    "GLX",
			Exponent: 6,
			Aliases:  []string{},
		},
	},
}

Are these 2 separate tokens? It seems that they are which to me is very confusing.

So, I think I understand this now after digging in some more. This metadata is more just a hint for extra info on how its base token can be displayed, divided, etc. For example, in wallets and other front ends.

Id love to see a better explanation of this honestly. Seems like setting up the base gov/staking token is one of the most important things one would do when starting a new chain.

Let me know if Im on the right track. :)

@alexanderbez
Copy link
Contributor

Which seems too still only be a proposal?

No, it's already implemented. It is utilized heavily by many components ;-)

What I'm trying to achieve is one single token with 6 decimal places that's used for gov/staking, etc. Im failing to see how this is possible so far. Do you have a recommendation of how I could achieve this?

I'm not really sure how to answer this. Also, this expands way beyond the scope of this issue which we're now polluting. You can have a token that is the staking and governance token. How many decimal places used is completely up to you. It's all defined in genesis.

@tac0turtle
Copy link
Member

closing this issue in favour of #11008

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

No branches or pull requests