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

x/tokenfactory #12

Merged
merged 25 commits into from
Sep 6, 2022
Merged

x/tokenfactory #12

merged 25 commits into from
Sep 6, 2022

Conversation

Reecepbcups
Copy link
Contributor

@Reecepbcups Reecepbcups commented Aug 27, 2022

close #11

I still don't know how to generate proto, so uses their namespace still for now via REST / RPC
http://localhost:1317/clockworkgr/tokenfactory/tokenfactory/denom

@Reecepbcups
Copy link
Contributor Author

Reecepbcups commented Aug 27, 2022

I think I got it, just the proto is still somehow pointing to the wrong URL path for the resp api, even though there are no occurances

I keep getting a could not find protoc plugin for name grpc-gateway error from buf.gen.gogo.yaml. Even though I am using github.com/grpc-ecosystem/grpc-gateway v1.16.0 like craft in my go.mod.
image

I have my go path setup correctly & protoc-gen-go-gprc installed
image

What am I missing here @vuong177 ?

@faddat
Copy link
Contributor

faddat commented Aug 27, 2022

hmmmm @nghuyenthevinh2000 is our finest with protobufs. Also, once this is complete it makes a ton of sense to take the same work and PR it to cosmwasm per:

many folks feel that tokenfactory is "the right way" and I'm one of them

@vuong177
Copy link
Contributor

vuong177 commented Aug 27, 2022

@Reecepbcups you can try

 go install \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2 \
    google.golang.org/protobuf/cmd/protoc-gen-go \
    google.golang.org/grpc/cmd/protoc-gen-go-grpc

@Reecepbcups
Copy link
Contributor Author

@Reecepbcups you can try

 go install \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2 \
    google.golang.org/protobuf/cmd/protoc-gen-go \
    google.golang.org/grpc/cmd/protoc-gen-go-grpc

yea I did that & finally got it from some cosmos-sdk issue / PR
issue now is that it still has other endpoint & not the updated one, so thats my next task to figure out thx

@Reecepbcups
Copy link
Contributor Author

hmmmm @nghuyenthevinh2000 is our finest with protobufs. Also, once this is complete it makes a ton of sense to take the same work and PR it to cosmwasm per:

many folks feel that tokenfactory is "the right way" and I'm one of them

I agree with it as well, CW20 kinda meh when Cosmos bank is already so good

I feel I should convert it to be more like osmosis with the factory/address/denom most likely, will continue to look into as best approach

@faddat
Copy link
Contributor

faddat commented Aug 27, 2022

I think I support doing it exactly the osmosis-way.

Hope that we don't end up finding that this blocks us from using sdk v0.46.* due to something related to:

CosmWasm/wasmd#941

It might be worth pinging @fadeev from the ignite cli / starport team. as we can think of eve, structurally as "slim craft". She may be usable without too many changes as a template, as long as we can come to agreement on the cmd folder contents -- I do not want to wrap cmd the way that the ignite cli historically has.

@nghuyenthevinh2000
Copy link
Collaborator

@Reecepbcups
For proto generation, try to use this docker. It has all the protoc-gen binaries installed. No more hassle of figuring out the correct protoc-gen binaries to install.

https://github.com/cosmos/cosmos-sdk/blob/main/Makefile#L395

tendermintdev/sdk-proto-gen:v0.7

@Reecepbcups
Copy link
Contributor Author

Reecepbcups commented Aug 30, 2022

I think I support doing it exactly the osmosis-way.

Hope that we don't end up finding that this blocks us from using sdk v0.46.* due to something related to:

CosmWasm/wasmd#941

It might be worth pinging @fadeev from the ignite cli / starport team. as we can think of eve, structurally as "slim craft". She may be usable without too many changes as a template, as long as we can come to agreement on the cmd folder contents -- I do not want to wrap cmd the way that the ignite cli historically has.

So as I have looked into this, osmosis uses a lot of their SDK related things for testing & in their modified bankKeeper for denom data.
Also, has some really weird features like burn denom from any account

I took a more CW20 approach given that is what the factory is for, where the admin can only mint new tokens to an account, not modify others balances.

Need to add:

  • wasmbindings
  • move osmosis test over
    & it should be ready to go.

@faddat
Copy link
Contributor

faddat commented Sep 6, 2022

Propose that we revert the last commit then merge?

This reverts commit 131b049.
@faddat faddat marked this pull request as ready for review September 6, 2022 23:08
@faddat faddat merged commit c1748a7 into main Sep 6, 2022
@Reecepbcups
Copy link
Contributor Author

Propose that we revert the last commit then merge?

yea thanks Ill work on that later, need to clean up a few functions & will push on main soon
Definitely a need later so CW can use native denoms instead of CW20 (which are bleh for queries)

Have to do it the factory/addr/{denom} way so namespaces done merge
else someone could mint ueve & mint to anyone, just found that bug :P

@hoank101 hoank101 deleted the tokenfactory branch March 19, 2024 02:52
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.

x/TokenFactory module
4 participants