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

[SG-235] feat: integrate token factory #803

Merged
merged 26 commits into from
Jun 15, 2023
Merged

Conversation

spoo-bar
Copy link
Collaborator

@spoo-bar spoo-bar commented May 9, 2023

Closes: #700

Bringing in the Osmosis token factory (as of Osmosis v15.1.0) to Stargaze.

The module additionally includes the changes by @larry0x which allows the denom creation to consume gas if denom creation fee is set to zero. Ref: #803 (comment) & osmosis-labs/osmosis#4983

@linear
Copy link

linear bot commented May 9, 2023

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Patch coverage: 10.70% and project coverage change: +0.02 🎉

Comparison is base (dc574e4) 10.54% compared to head (e3f122b) 10.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
+ Coverage   10.54%   10.56%   +0.02%     
==========================================
  Files          98      119      +21     
  Lines       14227    17999    +3772     
==========================================
+ Hits         1500     1902     +402     
- Misses      12635    15967    +3332     
- Partials       92      130      +38     
Impacted Files Coverage Δ
app/upgrades.go 11.76% <0.00%> (-4.91%) ⬇️
x/tokenfactory/types/keys.go 0.00% <0.00%> (ø)
x/tokenfactory/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/tokenfactory/types/tokenfactory.go 0.00% <0.00%> (ø)
x/tokenfactory/types/tx.pb.go 0.86% <0.86%> (ø)
x/tokenfactory/types/query.pb.go 1.10% <1.10%> (ø)
x/tokenfactory/types/tokenfactory.pb.go 1.40% <1.40%> (ø)
x/tokenfactory/types/genesis.pb.go 2.68% <2.68%> (ø)
x/tokenfactory/types/params.go 32.14% <32.14%> (ø)
x/tokenfactory/types/codec.go 50.00% <50.00%> (ø)
... and 13 more

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

@faddat
Copy link
Contributor

faddat commented May 10, 2023

Hey, I will ask @larry0x if this is a good implementation. Scary piece about the token factory is that there are so many versions. @Reecepbcups did a pretty nice one for Juno though and I'm going to promote it.

@larry0x
Copy link

larry0x commented May 10, 2023

Hey, I will ask @larry0x if this is a good implementation. Scary piece about the token factory is that there are so many versions. @Reecepbcups did a pretty nice one for Juno though and I'm going to promote it.

This PR seems to be simply copy-pasting code in the Osmosis repo over. This is probably the better solution over importing Confio or Juno's fork imo, because when Osmosis finally spins off their module into a separate repo you can simply delete the code copied over and import that repo instead. If using a forked version I'd be worried about incompatibilities.

A few suggestions:

  1. I recently made a contribution to Osmosis introducing a "denom creation gas consume" feature. The rationale for using this over the "denom creation fee" is discussed here. Do you mind including this in your PR as well? (This doesn't work for Stargaze which has zero gas price, but for the purpose compatibility I'd suggest we keep the code as close to Osmosis' as possible.)

  2. I think you need an upgrade handler to initialize the module's parameters. See here for an example.

  3. This PR doesn't seem to include any wasm bindings. In this case, can you enable Stargate queries so that contracts can interact with the module? See here for an example.

I'm happy to PRs on these if you prefer (i.e. I will make PRs targeting your branch)

@spoo-bar
Copy link
Collaborator Author

Thanks for the feedback on the PR @larry0x

  1. Will check out the "denom creation gas consume" feature as Stargaze is implementing globalfees with the upcoming release (v10) as well.
  2. Upgrade handler has intentionally not been implemented yet as token factory is planned for v11 (so upgrade after the next upgrade).
  3. Had missed the Stargate queries enabling. Thanks for the catch 👍🏻

@faddat
Copy link
Contributor

faddat commented May 10, 2023

seems we're on the same page on most of this.

Thanks very much for your review Larry.

to my knowledge no one knows exactly what to do about token factory. Maybe I should open an issue on osmo and try to get juno changes upstreamed.

I think both osmo and juno hoped to have it in wasmd as a first class citizen, and we (notional) maintain this thing:

https://github.com/notional-labs/wasmd/releases/tag/v0.40.0-tf.rc4

but tbh the more I think about it the more it feels like some sort of monstrosity.

I like the modularity idea, however, there's just not a canonical source. @Reecepbcups tried to make one (valiant effort) github.com/cosmostokenfactory -- but then there were maintenance issues wrt juno.

Tbh I am mildly out of ideas.

@larry0x
Copy link

larry0x commented May 10, 2023

I think both osmo and juno hoped to have it in wasmd as a first class citizen

I think it doesn't necessarily need to be in wasmd, but definitely has to be a separate repo

Sunny seems to want it upstreamed to cosmos-sdk repo, which would take muuuuuch longer to happen

Please push for the spin off to happen Jacob

@faddat
Copy link
Contributor

faddat commented May 10, 2023

@larry0x -- spinoff happened and was cancelled, imo for good reason. I like this idea of putting it in cosmos-sdk tbh. A lot. I would need to see if I can muster the troops to refactor it tho.

@spoo-bar spoo-bar requested a review from jhernandezb May 25, 2023 12:51
@spoo-bar spoo-bar marked this pull request as ready for review May 25, 2023 12:51
@jhernandezb jhernandezb merged commit b5e56da into main Jun 15, 2023
7 checks passed
@spoo-bar spoo-bar deleted the spoorthi/token-factory branch June 15, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Token Factory module
4 participants