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

feat: mint large amount of coins(gov mint) #709

Merged
merged 16 commits into from
Oct 17, 2022

Conversation

dudong2
Copy link
Contributor

@dudong2 dudong2 commented Oct 12, 2022

Description

It is necessary to mint a large amount of coins.
For this, x/foundation provides a minting function(one time mint) through proposals.
However, this function is limited to only 1 time.

closes: #XXXX

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@dudong2 dudong2 self-assigned this Oct 12, 2022
@dudong2 dudong2 added the A: improvement Changes in existing functionality label Oct 12, 2022
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #709 (1aeab8b) into main (289ca6b) will increase coverage by 0.09%.
The diff coverage is 89.24%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   61.83%   61.93%   +0.09%     
==========================================
  Files         874      874              
  Lines       98557    98776     +219     
==========================================
+ Hits        60944    61177     +233     
+ Misses      33982    33965      -17     
- Partials     3631     3634       +3     
Impacted Files Coverage Δ
simapp/app.go 86.14% <ø> (ø)
x/foundation/codec.go 0.00% <0.00%> (ø)
x/foundation/keeper/grpc_query.go 4.10% <0.00%> (-0.18%) ⬇️
x/foundation/keeper/keys.go 100.00% <ø> (ø)
x/foundation/keeper/msg_server.go 80.43% <68.00%> (-1.52%) ⬇️
x/foundation/client/testutil/query.go 100.00% <100.00%> (ø)
x/foundation/client/testutil/suite.go 97.91% <100.00%> (+0.02%) ⬆️
x/foundation/client/testutil/tx.go 99.29% <100.00%> (+0.05%) ⬆️
x/foundation/genesis.go 76.41% <100.00%> (+0.68%) ⬆️
x/foundation/keeper/genesis.go 80.26% <100.00%> (+0.39%) ⬆️
... and 12 more

Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the current design, OneTimeMintMaxCount has the max count of the mints.

  • One executes the minting, then the count is decreased. I think it must not change the value, as the name of value suggests.
  • When the value is exported, it may have a zero-value, which will throw an error in the next chain initiation.

Therefore, I suggest you change the name of the value (e.g. NumberOfMintingLeft), which would mean the number of times you have left, and set the default value to zero. And modify the corresponding codes as the name suggests.

x/foundation/msgs.go Outdated Show resolved Hide resolved
x/foundation/keeper/genesis.go Outdated Show resolved Hide resolved
x/foundation/keeper/msg_server.go Outdated Show resolved Hide resolved
@dudong2 dudong2 requested a review from 0Tech October 13, 2022 17:03
Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think the updated design has no flaws.
One side note: Can you be sure that this "one time" spec won't change? If there is even the slightest chance of such a case, the names of the relevant variables should be changed, and also the literal should be replaced by a config.

x/foundation/genesis.go Outdated Show resolved Hide resolved
x/foundation/msgs.go Show resolved Hide resolved
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using GovMint or governanceMint the value name instead of OneTimeMint

@@ -50,6 +50,9 @@ service Msg {
// Revoke revokes any authorization corresponding to the provided method name on the
// granter that has been granted to the grantee.
rpc Revoke(MsgRevoke) returns (MsgRevokeResponse);

// OneTimeMint defines a one-time mint coins to the treasury.
rpc OneTimeMint(MsgOneTimeMint) returns (MsgOneTimeMintResponse);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add cli and cli unittest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of one-time mint, it is not appropriate to expose it to the cli because it should be performed only through proposals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other proposal tx also has cli and cli test. For example, WithdrawFromTreasury and Grant. Check it please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added cli and cli test. Thank you.

@zemyblue
Copy link
Member

Please update swagger and statik file.

@0Tech 0Tech self-requested a review October 14, 2022 08:48
Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention query.proto. You need to add the corresponding endpoint to show the number of minting left.

@dudong2 dudong2 requested a review from 0Tech October 16, 2022 23:19
@dudong2 dudong2 requested a review from zemyblue October 16, 2022 23:19
proto/lbm/foundation/v1/query.proto Outdated Show resolved Hide resolved
x/foundation/genesis.go Outdated Show resolved Hide resolved
proto/lbm/foundation/v1/query.proto Outdated Show resolved Hide resolved
@@ -37,6 +37,9 @@ message GenesisState {

// pool
Pool pool = 8 [(gogoproto.nullable) = false];

// govMintLeftCount is one time mint max count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing one time mint comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed all one time mint to gov mint.

x/foundation/client/testutil/cli_test.go Show resolved Hide resolved
@dudong2 dudong2 changed the title feat: one time mint feat: gov mint Oct 17, 2022
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update statik.go file also. And could you change the title more easier to understand about this changes?

@dudong2 dudong2 changed the title feat: gov mint feat: mint large amount of coins(gov mint) Oct 17, 2022
@zemyblue
Copy link
Member

Please update statik.go file also. And could you change the title more easier to understand about this changes?

My system's statik.go is a little difference with upstream. But the code is not difference, just comment and new line. So it is not problem. Sorry.

@dudong2 dudong2 merged commit 66988a2 into Finschia:main Oct 17, 2022
@dudong2 dudong2 mentioned this pull request Oct 26, 2022
5 tasks
@dudong2 dudong2 deleted the dudong2/feat/one-time-LN-mint branch October 26, 2022 14:33
@zemyblue zemyblue mentioned this pull request Oct 27, 2022
5 tasks
@zemyblue zemyblue mentioned this pull request Nov 28, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: improvement Changes in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants