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 the Minter struct from genesis #2067

Closed
rach-id opened this issue Jul 10, 2023 · 2 comments · Fixed by #2217
Closed

Remove the Minter struct from genesis #2067

rach-id opened this issue Jul 10, 2023 · 2 comments · Fixed by #2217
Assignees
Labels
audit item is from an audit

Comments

@rach-id
Copy link
Member

rach-id commented Jul 10, 2023

Context

// InitGenesis initializes the x/mint store with data from the genesis state.
func (k Keeper) InitGenesis(ctx sdk.Context, ak types.AccountKeeper, data *types.GenesisState) {
k.SetMinter(ctx, data.Minter)
// override the genesis time with the actual genesis time supplied in `InitChain`
blockTime := ctx.BlockTime()
gt := types.GenesisTime{
GenesisTime: &blockTime,
}
k.SetGenesisTime(ctx, gt)

Problem

Currently, the minter struct is exposed in the genesis file:

    "mint": {
      "minter": {
        "inflation_rate": "0.080000000000000000",
        "annual_provisions": "0.000000000000000000",
        "previous_block_time": null,
        "bond_denom": "utia"
      }
    },

None of these fields is not used in the app:

  • inflation_rate: Even if a value is set at this level, the inflation rates are hard-coded and these values are not used:

    newInflationRate := minter.CalculateInflationRate(ctx, *genesisTime)

  • annual_provisions: Also calculated in BeginBlocker in the first block:

    minter.AnnualProvisions = newInflationRate.MulInt(totalSupply)

  • previous_block_time: Already not specified in genesis

  • bond_denom: This parameter is not expected to change at all

Proposal

Remove the Minter struct from genesis.

We could also remove the protobuf definition (To be confirmed) as the Minter struct will only be stored in state, and not written to genesis, or used in tx/queries

@rach-id rach-id added audit item is from an audit x/mint labels Jul 10, 2023
@rootulp
Copy link
Collaborator

rootulp commented Aug 5, 2023

I think we should preserve bond_denom in the genesis for consistency with other modules (i.e. x/staking here, x/gov here). I agree, the other fields should be removed from the genesis file because they are overwritten at block one anyway. Unfortunately, I think this may be a breaking change that is too late to include in the v1.0.0 release because it implies a change to the genesis.json.

@rootulp rootulp self-assigned this Aug 5, 2023
@rootulp rootulp added this to the Post-mainnet milestone Aug 5, 2023
@rach-id
Copy link
Member Author

rach-id commented Aug 5, 2023

I agree, but concerning this:

Unfortunately, I think this may be a breaking change that is too late to include in the v1.0.0 release because it implies a change to the genesis.json.

Even if we remove those fields from the struct, or even remove the whole Minter struct, the app won't complain if fed a genesis file containing those fields. So we can keep the same genesis while having this issue implemented

rootulp added a commit that referenced this issue Aug 14, 2023
Closes #2067

Before this PR, genesis.json looked like:

```json
    "mint": {
      "minter": {
        "inflation_rate": "0.080000000000000000",
        "annual_provisions": "0.000000000000000000",
        "previous_block_time": null,
        "bond_denom": "utia"
      }
    },
```

After this PR, genesis.json should be:

```json
    "mint": {
      "bond_denom": "utia"
    },
```
mergify bot pushed a commit that referenced this issue Aug 14, 2023
Closes #2067

Before this PR, genesis.json looked like:

```json
    "mint": {
      "minter": {
        "inflation_rate": "0.080000000000000000",
        "annual_provisions": "0.000000000000000000",
        "previous_block_time": null,
        "bond_denom": "utia"
      }
    },
```

After this PR, genesis.json should be:

```json
    "mint": {
      "bond_denom": "utia"
    },
```

(cherry picked from commit 92be1fc)
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Closes celestiaorg/celestia-app#2067

Before this PR, genesis.json looked like:

```json
    "mint": {
      "minter": {
        "inflation_rate": "0.080000000000000000",
        "annual_provisions": "0.000000000000000000",
        "previous_block_time": null,
        "bond_denom": "utia"
      }
    },
```

After this PR, genesis.json should be:

```json
    "mint": {
      "bond_denom": "utia"
    },
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit item is from an audit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants