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

Token Factory module #584

Closed
wants to merge 27 commits into from
Closed

Token Factory module #584

wants to merge 27 commits into from

Conversation

sunnya97
Copy link
Collaborator

@sunnya97 sunnya97 commented Nov 15, 2021

The token factory module allows any account to start minting coins. Denoms are namespaced by the coin creator.

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2021

Codecov Report

Merging #584 (b119d48) into main (84f0194) will decrease coverage by 1.32%.
The diff coverage is 7.57%.

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   19.81%   18.49%   -1.33%     
==========================================
  Files         210      219       +9     
  Lines       27884    30146    +2262     
==========================================
+ Hits         5525     5575      +50     
- Misses      21333    23544    +2211     
- Partials     1026     1027       +1     
Impacted Files Coverage Δ
x/tokenfactory/types/codec.go 0.00% <0.00%> (ø)
x/tokenfactory/types/keys.go 0.00% <0.00%> (ø)
x/tokenfactory/types/msgs.go 0.00% <0.00%> (ø)
x/tokenfactory/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/tokenfactory/types/tx.pb.go 0.81% <0.81%> (ø)
x/tokenfactory/types/query.pb.go 0.97% <0.97%> (ø)
x/tokenfactory/types/authorityMetadata.pb.go 1.09% <1.09%> (ø)
x/tokenfactory/types/genesis.pb.go 2.03% <2.03%> (ø)
x/tokenfactory/keeper/bankactions.go 42.30% <42.30%> (ø)
x/tokenfactory/module.go 55.55% <55.55%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84f0194...b119d48. Read the comment docs.

@sunnya97 sunnya97 changed the title Sunny/mint coins Token Factory module Nov 21, 2021
@sunnya97 sunnya97 marked this pull request as ready for review November 22, 2021 01:32
@daniel-farina daniel-farina added this to the 2021 - December Milestone milestone Dec 2, 2021
@ValarDragon ValarDragon modified the milestones: 2021 - December Milestone, 2022 - January Milestone Dec 6, 2021
@daniel-farina daniel-farina removed this from the 2022-01 Milestone milestone Dec 24, 2021
app/app.go Outdated Show resolved Hide resolved
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM on first pass! Will do a second pass soon, then should be good to merge with a few follow-on issues.

Use: types.ModuleName,
Short: fmt.Sprintf("Querying commands for the %s module", types.ModuleName),
DisableFlagParsing: true,
SuggestionsMinimumDistance: 2,
Copy link
Member

Choose a reason for hiding this comment

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

oh interesting, maybe we can add this to more cmds

x/tokenfactory/keeper/keeper.go Show resolved Hide resolved
x/tokenfactory/module.go Outdated Show resolved Hide resolved
x/tokenfactory/types/denoms.go Outdated Show resolved Hide resolved
x/tokenfactory/types/denoms.go Show resolved Hide resolved
x/tokenfactory/types/denoms.go Show resolved Hide resolved
x/tokenfactory/types/keys.go Outdated Show resolved Hide resolved
sunnya97 and others added 2 commits December 25, 2021 01:39
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
sunnya97 and others added 2 commits December 25, 2021 11:22
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@sunnya97
Copy link
Collaborator Author

sunnya97 commented Dec 25, 2021

Should we move this authority metadata system to the bank module itself? @ValarDragon wdyt?

@ValarDragon
Copy link
Member

ValarDragon commented Dec 25, 2021

I think mint/burn permissions for a token do generally need to be rethought much more, and get into bank natively. I'm not fully sure how to square different approaches to this. This approach feels too restrictive for many general tokens (e.g. osmo, where we want many different modules to get mint perms -- Superfluid is in this set).

I was suggesting this approach as a non-state breaking approach to add more protections cosmos/cosmos-sdk#10386 . I think something cool would be to get this module to emit a "FactoryMintRestrictions" and "FactoryBurnRestrictions" functions, that we then compose into that approach. (Rather than need this authorization lists in bank itself. I actually like that this module is specifying its own permissioning schema!)

(We should also use that approach to restrict factory module perms)

The status of that is its in a PR thats imo good to merge pending spec update. (cosmos/cosmos-sdk#10771). Unclear what SDK team's thoughts on it are though. I think we should go ahead and just use that even if the SDK doesn't want to. (It solves having to worry about gamm module minting non-LP shares for instance, and is imo critical for superfluid)

@faddat
Copy link
Member

faddat commented Dec 26, 2021

I really like how this approach doesn't require the use of a contract per token, which is a broken pattern.

@daniel-farina daniel-farina added the T:task ⚙️ A task belongs to a story label Mar 1, 2022
@ValarDragon ValarDragon mentioned this pull request Mar 3, 2022
6 tasks
@ValarDragon ValarDragon mentioned this pull request Mar 29, 2022
4 tasks
@github-actions github-actions bot added the C:CLI label Apr 21, 2022
@sunnya97
Copy link
Collaborator Author

Rebased this to main, as well as

  1. Removed ForceTransfers
  2. Only allowed Mint and Burns to/from the admin address

@sunnya97
Copy link
Collaborator Author

sunnya97 commented May 1, 2022

Closing because has been replaced by other PRs

@sunnya97 sunnya97 closed this May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI T:task ⚙️ A task belongs to a story
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants