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

Add tokenfactory module #911

Closed
JakeHartnell opened this issue Jul 21, 2022 · 21 comments
Closed

Add tokenfactory module #911

JakeHartnell opened this issue Jul 21, 2022 · 21 comments
Labels
discussion M Estimated medium wontfix This will not be worked on

Comments

@JakeHartnell
Copy link

Inspired by this Twitter thread: https://twitter.com/larry0x/status/1550231427161227265

The tokenfactory module has been growing on me, and as @larry0x suggests:

the best way to promote the adoption of tokenfactory is to make it a part of wasmd, so any chain to include wasm automatically gets it

I think this would be great!

@faddat
Copy link
Contributor

faddat commented Aug 25, 2022

I think this would be great, too.

I think that doing token factory, osmosis-style here, as mentioned by Larry, is simply a great idea and something that we should be doing.

Can anyone advise on the best path forward for integrating tokenfactory?

@alpe alpe moved this to 🆕 New in wasmd backlog Sep 23, 2022
@alpe alpe moved this from 🆕 New to 📋 Unsorted Backlog in wasmd backlog Sep 23, 2022
@alpe alpe added the M Estimated medium label Sep 29, 2022
@alpe
Copy link
Contributor

alpe commented Sep 30, 2022

The code is owned and maintained by the Osmosis team. We should discuss a path with them to make it available that respects their past and future work.
It can also be a dependency in go.mod and set it up in the example app.go.

@onivalidator
Copy link

@alpe To play devil's advocate here - it seems to me that we sometimes get bogged down in the semantics of including the original contributors and what have you (who may also have differing views on future development). If we can fork tokenfactory in and the ones submitting the PR can competently maintain it, then why wouldn't we move forward in the name of development velocity and progress? Notional is a core contributor over there, so there's no finite reason I can see not to move this forward.

I hate to see development slowed by convoluted team discussions that may or may not resolve on any kind of finite timeline - even though I understand where you are coming from. Perhaps my own ignorance here is showing - but I'm a velocity maxi.

@jhernandezb
Copy link
Contributor

jhernandezb commented Oct 1, 2022

The main issue I see is that this repository is a reference implementation when it comes to app.go not the module itself, teams probably should importtokenfactory directly into their chains, now if custom bindings are needed that's probably a different conversation to have. Osmosis uses their own sdk fork, by importing it we will have conflicts with sdk versions and wasmd being an upstream dependency will cause troubles for many projects depending on it.

Maybe tokenfactory should be extracted to its own repo which should resolve sdk dependency issues, but still remains optional for chains that wants it and not directly into this repo.

@onivalidator
Copy link

This seems like a great solution on all fronts, if there's conflicts with the SDK version. Who can we chat up to shop this idea? Seems like tokenfactory in it's own repo is the cleanest and fastest solution for all.

@ethanfrey
Copy link
Member

Maybe tokenfactory should be extracted to its own repo which should resolve sdk dependency issues, but still remains optional for chains that wants it and not directly into this repo.

yes, I would love to make this it's own repo. one for the go bindings (just import and add two lines to app.go) and another for the rust bindings (that can be used for any chain).

I wanted to do this for the last month, but got too many other demands. Totally happy to make a new CosmWasm/tokenfactory and CosmWasm/tokenfactory-bindings repos

@jhernandezb
Copy link
Contributor

Happy to take the lead on this if you want, Stargaze is planning on adding it soon as well.

@sunnya97
Copy link
Contributor

sunnya97 commented Oct 1, 2022

We are going through an audit of tokenfactory with Informal Systems. Would recommend waiting for that to finish before upstreaming

@onivalidator
Copy link

Thanks for the heads up Sunny. Able to share any ETA on the audit completion or approx date of completion?

@alpe
Copy link
Contributor

alpe commented Oct 6, 2022

@jhernandezb 👍
@sunnya97 👍 thanks for the update.

@onivalidator I do understand the demand to use the module but as discussed, merging the code into this project and
maintaining it is only one option. It seems simple but it does not scale well. Looking forward into a future with many great custom extensions, a different model is required.
Beside implementing new features, my focus was to make the project customizable and extendable. Cosmos based chains have a model already to integrate modules from different repos. We should aim for the same. Have a process, doc, tests and tooling. The tokenfactory is the first module to get this started, IMHO

@ethanfrey
Copy link
Member

Please see https://github.com/CosmWasm/token-factory and https://github.com/CosmWasm/token-bindings which do this as an external repo. This "standardises" CustomMsg and CustomQuery and the module that can be shared by multiple chains, without requiring changes in wasmd or CosmWasm.

This is also a model how other people can make standards without blocking on Confio

@faddat
Copy link
Contributor

faddat commented Mar 1, 2023

It's really pretty surprising that Confio feels that it can stop development on ciritical software, and expect that there will not be rumors caused by that.

Effective leadership would be to openly discuss issues.

I've also made an adjustment to the code of conduct to reflect the reality of this repository

@alpe alpe removed the blocked label Mar 3, 2023
@alpe
Copy link
Contributor

alpe commented Mar 3, 2023

@faddat not sure how your comment contributes to this feature request? Please keep a professional tone

As outlined in #911 (comment) , embedding the token-factory is only one option. You may have a different opinion but pushing the code with any of your PRs is not helping the conversation or the project!

No chain is blocked on using the factory today already!

With Ethan's work the new repo setup, the foundation was led. I also saw the juno fork of this where you contributed. It is integrated into junod main as a Go module, which is a much better way for managing and maintaining a growing set of extensions compared to a mono-repo.

So why is this github issue marked blocked?

  • It was blocked by the code audit that @sunnya97 announced . Has anybody an update on this?
  • It was blocked on the missing repo for the extension, which Ethan solved
  • There was also somebody volunteering from the community to drive this feature forward so that this was not high on my list.

But I have removed the label now.

What is stopping us now to integrate the token factory extension as a Go module in wasmd?

  • Priorities! There is a lot of work in wasmd main already that is not released, yet. Next comes the sdk 47 integration.
  • The token-factory repo needs some love and maybe code updates from the osmosis repo
  • There is still no process defined how to structure extensions and how they can be evaluated and added to wasmd
  • Dependencies are a concern in extensions that should be considered. Splitting the repo into different Go modules may address this. A reference implementation would be helpful.

I hope this gives some answers. I would appreciate constructive feedback and discussion instead.

@faddat
Copy link
Contributor

faddat commented Mar 17, 2023

Hi, I was just quite surprised by commentary on working while confio was on strike, and being blamed for discussing the rumors created by the strike. That isn't my fault, and I feel that my PR was closed for frankly discussing the current state of development here.

I updated the code of conduct to reflect that prohibition on discussing roadmaps outside of confio.

I don't think my tone was unprofessional, maybe go have a reread of my exact words, thank you.

So let's begin. There's no well-maintained token factory anywhere. Not Juno's not Osmosis's (well, it is integrated into osmosis) and not confio's. I'd really love it if the three teams actively using token factory, could target token factory changes to one place, and that place would ideally be here.

#1214 is a PR that I've been keeping up to date with the 47 branch. Maybe we could merge it so that we could have a standard on the token factory?

The monorepo design is preferable because otherwise the maintenace work gets duplicated, as we are currently seeing in ibc-go.

Is just better than:

It also ensures that the token factory is attended to alongside wasmd. In my PR I made it so that it is still possible to use only the token factory, or only x/wasm. So, I'd like to use that, so that we as an ecosystem can standardize around the use of the token factory. I know zero contract authors who think TF is bad.

Also, I'm definitely doing the work for about the third time.

  • don't know any contract authors who don't want token factory
  • don't know any chain teams who don't want token factory
  • when authors and teams want token factory, they want it with wasmd

...so the idea is to ship the token factory from wasmd. For the public good.


Status of various token factory editions:

  • Juno probably want's osmosis' updates
  • Osmosis would likely like to stop maintaining a fork of the sdk, wasmd, and token factory, tho I cannot speak for the osmosis team, this is more of a feeling
  • white whale wants osmosis' updates
  • I don't know what confio wants

@faddat
Copy link
Contributor

faddat commented Apr 10, 2023

This continues to be painful:

If there's no intent to bring TF into wasmd, then I suggest that we close this issue.

@JakeHartnell
Copy link
Author

I still think this would be a good thing to add if the Confio team supports it. Would help ease some of the pain that came up in this thread: https://twitter.com/larry0x/status/1641096051837878280?s=20

@pinosu
Copy link
Contributor

pinosu commented Apr 11, 2023

fyi: Alex is on holidays and will be back next week.

@alpe
Copy link
Contributor

alpe commented Apr 17, 2023

@JakeHartnell thanks for highlighting the problems. I agree that we should standardise for a better dev UX. Priorities are still on SDK 47 support but we are on a good path

@faddat this issue is about adding the token module. I had outline the path before

@ethanfrey
Copy link
Member

I still think this would be a good thing to add if the Confio team supports it. Would help ease some of the pain that came up in this thread: https://twitter.com/larry0x/status/1641096051837878280?s=20

The issue has nothing to do about being in wasmd or not.

In theory Osmosis and Juno could have actually reviewed the work I did, upstreamed any changes, and integrated that shard repo into both repos.

In practice, Osmosis kept working on their original version (which makes sense for them)
Juno forked the repo I started (without commenting on this) and released in v14.
I don't think there was any discussion between the two chains about standardizing and upstreaming (or reverting) some of the API changes I made.

We need less stuff in wasmd (where Confio has to push it to everyone).
More stuff in a shared repo where the interested parties (Juno, Osmosis, Stargaze, ???) can collaborate.
I was just trying to facilitate this as Confio, but I cannot force any code on any chain.

I can discuss this more tomorrow on the Community Call which you are more than welcome to join

@alpe
Copy link
Contributor

alpe commented Apr 19, 2023

Update: we are not going to add the the token factory to wasmd nor maintain the token-factory project. Please see the recording ~16:50

I can keep this issue open for discussion for some time

@alpe alpe added the wontfix This will not be worked on label Apr 19, 2023
@faddat
Copy link
Contributor

faddat commented May 13, 2023

No it's okay, I figure this is better off closed :) <3

Thanks for explaining your reasoning in such detail, it's appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M Estimated medium wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants