Update mint module #273

dimiandre · 2022-08-24T21:29:04Z

Halving time now is based on the total supply instead of fixed blocks per year param.
Better round block provisions during phase changes

--
Need to update tests

* Halving time now is based on the total supply instead of fixed blocks per year param. * Better round block provisions during phase changes

x/mint/abci.go


+	// fetch current total supply
+	totalSupply := k.TokenSupply(ctx, params.MintDenom)


x/mint/abci.go

 		minter.Inflation = newInflation
 		minter.Phase = nextPhase
 		minter.StartPhaseBlock = currentBlock
 		minter.AnnualProvisions = minter.NextAnnualProvisions(params, totalSupply)
+		minter.TargetSupply = totalSupply.Add(minter.AnnualProvisions.TruncateInt())


x/mint/abci.go

 		minter.Inflation = newInflation
 		minter.Phase = nextPhase
 		minter.StartPhaseBlock = currentBlock
 		minter.AnnualProvisions = minter.NextAnnualProvisions(params, totalSupply)
+		minter.TargetSupply = totalSupply.Add(minter.AnnualProvisions.TruncateInt())


x/mint/abci.go

 		// inflation phase end
 		if minter.Inflation.Equal(sdk.ZeroDec()) {
 			return
 		}
 	}

 	// mint coins, update supply
-	mintedCoin := minter.BlockProvision(params)
+	mintedCoin := minter.BlockProvision(params, totalSupply)


nullmames · 2022-08-25T05:29:43Z

I have read through and it appears that the intent from Core discussions has been captured. I have no comment on code.

the-frey · 2022-08-25T09:10:41Z

Had a quick scan. Approach looks good (will read more closely later). Looks like the linter is right, there's some error handling needed?

faddat · 2022-08-25T15:39:57Z

@dimiandre do you mind if I work on this branch directly?

dimiandre · 2022-08-25T16:56:55Z

@dimiandre do you mind if I work on this branch directly?

I think it's fine! We can then merge and base the other things we have to push on this

faddat · 2022-08-25T17:01:05Z

I think I'd start with the other things first, since they're larger changes, but either way--

Here's a small piece of how we (notional) think of blockchain development-- this PR here is a feature, a deviation from the "base" while

#262

is "base".

We usually go base -> features but I'm good with either direction in this case :).

faddat · 2022-08-25T17:03:22Z

regarding the codeql findings-- basically, the only other way to handle these, is to use a blank import, but the effect is the same: it is a dangerous area of the code.

dimiandre · 2022-08-25T17:39:53Z

I think I'd start with the other things first, since they're larger changes, but either way--

Here's a small piece of how we (notional) think of blockchain development-- this PR here is a feature, a deviation from the "base" while

#262

is "base".

We usually go base -> features but I'm good with either direction in this case :).

I prefer your approach more as well, I can rebase this branch on that one.

Let me know

faddat · 2022-08-25T19:08:24Z

Ok, if you'd like to go that way, basically first step is that we'd get #262 merged, and then we should get an "update branch" option here and shouldn't require any additional lifting beyond that :)

dimiandre · 2022-08-29T18:07:29Z

Testing again on V10, and will start working on upgrade handler.

dimiandre · 2022-09-01T10:58:53Z

All test in my local environment successful.

Need to write more unit test

faddat · 2022-09-03T10:49:14Z

We've got a team member who does amazing unit tests, I'll see if he has any capacity for this.

x/mint/types/minter_test.go

x/mint/types/minter.go

the-frey · 2022-09-05T17:13:29Z

Need to write upgrade handler to calculate "TargetSupply"

This bit is done now, right?

dimiandre · 2022-09-05T17:42:12Z

Need to write upgrade handler to calculate "TargetSupply"

This bit is done now, right?

yep!

x/mint/abci.go

+	totalSupply := k.TokenSupply(ctx, params.MintDenom)
+
+	// check if we need to change phase
+	nextPhase := minter.NextPhase(params, totalSupply)


giansalex · 2022-09-07T05:14:24Z

what will happen if coins are burned? e.g. whale funds.

the-frey · 2022-09-07T09:50:27Z

@giansalex I guess it will mint more until it hits totalSupply and changes phase? Or is that not right @dimiandre because totalSupply isn't total all-time supply, but instead total supply in this phase, in which case it will just continue as it would have done, and the total all-time supply of Juno will decrease.

nullmames · 2022-09-07T10:02:51Z

I guess it depends on if the burn actually removes tokens from bank.

Is our version of a burn sending to a non existing account or are they actually removed from the bank module?

This issue was actually raised on the weekend on a twitter spaces too.

the-frey · 2022-09-07T10:29:25Z

CosmWasm burn, so tokens disappear, removed from circulating supply AFAICR. I will look up the impl later

giansalex · 2022-09-07T17:21:36Z

I guess it will mint more until it hits totalSupply and changes phase?

yes, but technically more tokens will be minted, burned funds will return to token supply in the future via x/mint module.
loses the point of burning tokens, especially in Juno which has a fixed max supply.

the-frey · 2022-09-07T17:59:59Z

Right, so actually you're saying we should calculate the checkpoints ahead of time and stick to them rather than having it function statelessly? Otherwise a burn never actually affects supply in the way intended...

dimiandre · 2022-09-08T10:25:13Z

Right, so actually you're saying we should calculate the checkpoints ahead of time and stick to them rather than having it function statelessly? Otherwise a burn never actually affects supply in the way intended...

How do you calculate ahead of time?

The only checkpoints we can fix are

Target supply
Target dates

if we fix supply, we have this burn problem unless we modify the burn function to recalculate the target supply
if we fix dates, we will have a total random supply based on block times

IMO if a burn event happens, we can just do an upgrade and modify annual provisions for that phase

faddat · 2022-09-08T19:34:54Z

Epochs?

2 options:

Osmosis epoch module is now importable. I prefer it to the x/epoching in the sdk
Backport x/epoching to 45

Sorry I didn't think of this earlier.

faddat

Approved, as I think we should deploy a testnet with this. I also think that we may wish to consider epochs.

Should we have a call about this today/monday?

the-frey · 2022-09-09T11:46:35Z

I'm now leaning into the opinion that the fact it will make itself back to the correct amount, releasing the difference to stakers is actually... kind of fine? If that changes, we can add additional logic or an upgrade in future to alter.

faddat · 2022-09-11T04:05:40Z

Kind of fine, is imo exactly what we are shooting for here. I basically won't trust this code until it's been on a testnet, so, lgtm!

<3

(NB: I don't think there's anything wrong with it. I just want to observe it in its natural habitat some more)

Update mint module

d3fc4be

* Halving time now is based on the total supply instead of fixed blocks per year param. * Better round block provisions during phase changes

github-advanced-security bot found potential problems Aug 24, 2022

View reviewed changes

faddat added 2 commits August 25, 2022 23:34

lints

f734102

total supply....

3bd7929

more tests

f450ecc

dimiandre and others added 6 commits August 25, 2022 21:36

fix tests

4db15ef

fix genesis test and decoder test

8e0d3e7

superlinter settings

514eb52

lint proto

f238b07

ignore generated files with superlinter

98b7e89

Merge branch 'main' into dimi/inflation

6afbd2a

faddat changed the title ~~WIP: Update mint module~~ WIP: Update mint module & overclock juno Aug 27, 2022

dimiandre added 2 commits August 31, 2022 21:02

add migration for mint module

39e4895

change upgrade name, calculate target supply

e5c5f17

the-frey reviewed Sep 5, 2022

View reviewed changes

x/mint/types/minter_test.go Show resolved Hide resolved

the-frey reviewed Sep 5, 2022

View reviewed changes

x/mint/types/minter_test.go Outdated Show resolved Hide resolved

the-frey reviewed Sep 5, 2022

View reviewed changes

x/mint/types/minter.go Show resolved Hide resolved

Remove extra logging, add extra test case and a couple of comments

886b8c8

github-advanced-security bot found potential problems Sep 5, 2022

View reviewed changes

the-frey requested a review from giansalex September 5, 2022 21:06

Hopefully shut the linter up

78d3b6a

the-frey requested review from faddat, the-frey and joeabbey September 5, 2022 21:43

faddat approved these changes Sep 8, 2022

View reviewed changes

the-frey approved these changes Sep 9, 2022

View reviewed changes

the-frey changed the title ~~WIP: Update mint module & overclock juno~~ Update mint module Sep 9, 2022

faddat merged commit 9480914 into main Sep 11, 2022

the-frey deleted the dimi/inflation branch September 11, 2022 11:17

This was referenced Sep 22, 2022

Lowering Juno's block times #269

Closed

The Bitcoin Flaw: Juno's Mint Module Upgrade #271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update mint module #273

Update mint module #273

dimiandre commented Aug 24, 2022 •

edited

Loading

nullmames commented Aug 25, 2022

the-frey commented Aug 25, 2022 •

edited

Loading

faddat commented Aug 25, 2022

dimiandre commented Aug 25, 2022

faddat commented Aug 25, 2022

faddat commented Aug 25, 2022

dimiandre commented Aug 25, 2022

faddat commented Aug 25, 2022

dimiandre commented Aug 29, 2022

dimiandre commented Sep 1, 2022

faddat commented Sep 3, 2022

the-frey commented Sep 5, 2022

dimiandre commented Sep 5, 2022

giansalex commented Sep 7, 2022

the-frey commented Sep 7, 2022

nullmames commented Sep 7, 2022 •

edited

Loading

the-frey commented Sep 7, 2022

giansalex commented Sep 7, 2022 •

edited

Loading

the-frey commented Sep 7, 2022

dimiandre commented Sep 8, 2022

faddat commented Sep 8, 2022

faddat left a comment •

edited

Loading

the-frey commented Sep 9, 2022

faddat commented Sep 11, 2022


		// fetch current total supply
		totalSupply := k.TokenSupply(ctx, params.MintDenom)

Update mint module #273

Update mint module #273

Conversation

dimiandre commented Aug 24, 2022 • edited Loading

nullmames commented Aug 25, 2022

the-frey commented Aug 25, 2022 • edited Loading

faddat commented Aug 25, 2022

dimiandre commented Aug 25, 2022

faddat commented Aug 25, 2022

faddat commented Aug 25, 2022

dimiandre commented Aug 25, 2022

faddat commented Aug 25, 2022

dimiandre commented Aug 29, 2022

dimiandre commented Sep 1, 2022

faddat commented Sep 3, 2022

the-frey commented Sep 5, 2022

dimiandre commented Sep 5, 2022

giansalex commented Sep 7, 2022

the-frey commented Sep 7, 2022

nullmames commented Sep 7, 2022 • edited Loading

the-frey commented Sep 7, 2022

giansalex commented Sep 7, 2022 • edited Loading

the-frey commented Sep 7, 2022

dimiandre commented Sep 8, 2022

faddat commented Sep 8, 2022

faddat left a comment • edited Loading

Choose a reason for hiding this comment

the-frey commented Sep 9, 2022

faddat commented Sep 11, 2022

dimiandre commented Aug 24, 2022 •

edited

Loading

the-frey commented Aug 25, 2022 •

edited

Loading

nullmames commented Sep 7, 2022 •

edited

Loading

giansalex commented Sep 7, 2022 •

edited

Loading

faddat left a comment •

edited

Loading