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: implement ADR-22: Per-subspace token factory #1141

Merged
merged 74 commits into from
Jul 4, 2023

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented May 15, 2023

Description

Deps: #1174
This PR implements ADR-22: Per-subspace token factory.

Closes: DCD-316


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Patch coverage: 83.98% and project coverage change: +0.11 🎉

Comparison is base (75c2d04) 80.80% compared to head (9ece290) 80.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage   80.80%   80.92%   +0.11%     
==========================================
  Files         194      215      +21     
  Lines       17310    17972     +662     
==========================================
+ Hits        13987    14543     +556     
- Misses       2727     2821      +94     
- Partials      596      608      +12     
Impacted Files Coverage Δ
app/app_config.go 50.00% <ø> (ø)
x/tokenfactory/simulation/genesis.go 0.00% <0.00%> (ø)
x/tokenfactory/simulation/utils.go 0.00% <0.00%> (ø)
x/tokenfactory/types/authority_metadata.go 0.00% <0.00%> (ø)
x/tokenfactory/types/keys.go 0.00% <0.00%> (ø)
x/tokenfactory/types/query.go 0.00% <0.00%> (ø)
x/tokenfactory/types/denoms.go 57.50% <57.50%> (ø)
x/tokenfactory/types/codec.go 64.51% <64.51%> (ø)
x/tokenfactory/keeper/bankactions.go 76.00% <76.00%> (ø)
x/tokenfactory/types/genesis.go 78.57% <78.57%> (ø)
... and 13 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dadamu
Copy link
Contributor Author

dadamu commented May 15, 2023

@RiccardoM @manu0466 I have a question when implementing it, is it would be better that we fork the Cosmwasm/tokenfactory to our organization then import it instead of copy everything then modifying the logic?

Something like:

import "github.com/desmos-labs/token-factory/x/tokenfactory/keeper"

type OurTokenFactoryKeeper struct {
    wrapped keeper.Keeper
    ...otherProperties
}

I think it would be easier to update tokenfactory when upstream has changes. What do you think?

@RiccardoM
Copy link
Contributor

@dadamu I think forking here is unnecessary. We just need to add some checks that can be easily performed inside a custom MsgServer. No need to maintain our own custom version in my opinion. We can use their keeper, querier and everything. We just need a custom MsgServer implementation that is able to deal with subspaces

@dadamu
Copy link
Contributor Author

dadamu commented May 15, 2023

@RiccardoM Thanks for the answer, will change to import tokenfactroy from third party instead of copying them here..

@dadamu
Copy link
Contributor Author

dadamu commented May 16, 2023

@RiccardoM @manu0466 Unfortunately, tokenfactory from both CosmWasm and Osmosis does not support cosmos v0.47.x features. We should stop the implementation until it is updated by their teams.

In addition, Cosmwasm team decided to stop maintaining tokenfactory last month, my apologies that providing the expiry info. So the importing should be changed to osmos-labs/osmosis.
CosmWasm/wasmd#911 (comment)

@RiccardoM
Copy link
Contributor

@RiccardoM @manu0466 Unfortunately, tokenfactory from both CosmWasm and Osmosis does not support cosmos v0.47.x features. We should stop the implementation until it is updated by their teams.

What features are you talking about?

@dadamu
Copy link
Contributor Author

dadamu commented May 16, 2023

@RiccardoM Updating to 0.47.x, are many breaking changes, which leads the error when importing to desmos, say:

Currently Cosmos-SDK version of the latest osmosis v15.1.0 is v0.46.10, we should wait for its upgrade.

@RiccardoM
Copy link
Contributor

@dadamu What about implementing our token factory without relying on external implementations? And then we later change to replace that if we consider that to be worth.

@dadamu dadamu force-pushed the paul/DCD-316/token-factory-impl branch 5 times, most recently from 455394b to d56b577 Compare June 5, 2023 12:27
@github-actions github-actions bot added the x/CLI label Jun 6, 2023
@dadamu dadamu force-pushed the paul/DCD-316/token-factory-impl branch from cb85c53 to a4a020d Compare June 9, 2023 07:15
@github-actions github-actions bot added the x/subspaces Issue on the x/subspaces module label Jun 12, 2023
@dadamu
Copy link
Contributor Author

dadamu commented Jun 14, 2023

It seems MsgSetDenomMetadata breaks the validator bonding tokens export, I guess something wrong inside x/staking

Updated: June 14, 2023, 11:41:00 UTC +0
After checking the exported x/staking genesis file, it hits the special edge case that all validators are jailed in the simulation test. Fixed by updating new weight.

@dadamu dadamu force-pushed the paul/DCD-316/token-factory-impl branch from 89d8757 to 225e527 Compare June 14, 2023 11:49
@dadamu dadamu marked this pull request as ready for review June 14, 2023 12:46
@dadamu dadamu requested a review from a team as a code owner June 14, 2023 12:46
@dadamu dadamu force-pushed the paul/DCD-316/token-factory-impl branch from ae7d2c2 to ace0f7c Compare June 14, 2023 12:53
@dadamu dadamu marked this pull request as draft June 14, 2023 14:28
@dadamu dadamu force-pushed the paul/DCD-316/token-factory-impl branch 3 times, most recently from b9994c0 to e64f394 Compare June 15, 2023 08:29
@dadamu dadamu marked this pull request as ready for review June 15, 2023 10:50
@dadamu
Copy link
Contributor Author

dadamu commented Jun 15, 2023

@RiccardoM Ready to review

x/tokenfactory/types/codec.go Outdated Show resolved Hide resolved
@dadamu dadamu requested a review from RiccardoM July 4, 2023 06:37
dadamu and others added 13 commits July 4, 2023 15:36
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Great job! 💯

@RiccardoM RiccardoM merged commit c9d7b38 into master Jul 4, 2023
35 checks passed
@RiccardoM RiccardoM deleted the paul/DCD-316/token-factory-impl branch July 4, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x/CLI x/subspaces Issue on the x/subspaces module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants