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

Adding tokenfactory denom metadata endpoint #1444

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

dssei
Copy link
Contributor

@dssei dssei commented Mar 15, 2024

Describe your changes and provide context

When we create tokens using token factory, denoms are created in the factory/{owner_walltet_id}/{denom} format
E.g. factory/sei1gxskuzvhr4s8sdm2rpruaf7yx2dnmjn0zfdu9q/NEWCOIN

When querying metadata via /cosmos/bank/v1beta1/denoms_metadata/{denom} endpoint for such a denom we are getting error like

{
    "code": 12,
    "message": "Not Implemented",
    "details": []
}

This happens, because routing logic splits the denom uri param into 3 components rather then one and hence tries to router request to a non-existing handler. Url encoding does not help either as it still results in same issue. We would like to add additional endpoint to handle token factory created metadata retrieval, and move denom for the endpoint from uri param to query param.

so then for token factory created tokens the query would be
/sei-protocol/seichain/tokenfactory/denoms/metadata?denom=factory/{owner_walltet_id}/{denom}

Testing performed to validate your change

  • Functional testing on local node with updated source
  • unit/integration tests

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 62.89%. Comparing base (7ee257f) to head (fd78490).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
+ Coverage   62.64%   62.89%   +0.25%     
==========================================
  Files         257      257              
  Lines       16867    16883      +16     
==========================================
+ Hits        10566    10619      +53     
+ Misses       5798     5757      -41     
- Partials      503      507       +4     
Files Coverage Δ
x/tokenfactory/keeper/grpc_query.go 66.66% <81.25%> (+13.72%) ⬆️

... and 1 file with indirect coverage changes

proto/README.md Outdated
Your OS: darwin
Your arch: arm64
Your go version: go version go1.22.0 darwin/arm64
Your uname -a: Darwin Denyss-MacBook-Pro.local 23.1.0 Darwin Kernel Version 23.1.0: Mon Oct 9 21:32:11 PDT 2023; root:xnu-10002.41.9~7/RELEASE_ARM64_T6030 arm64

Choose a reason for hiding this comment

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

Do you want your username in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

Then, to generate the code, run the following command:

```bash
ignite generate proto-go

Choose a reason for hiding this comment

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

Is this for generating the proto files or for generating the openapi yml?

Copy link
Contributor Author

@dssei dssei Mar 15, 2024

Choose a reason for hiding this comment

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

Its for both, will add another readme for docs generation in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nicee!

go.sum Outdated
google.golang.org/genproto/googleapis/api v0.0.0-20240125205218-1f4bbc51befe h1:0poefMBYvYbs7g5UkjS6HcxBPaTRAmznle9jnxYoAI8=
google.golang.org/genproto/googleapis/api v0.0.0-20240125205218-1f4bbc51befe/go.mod h1:4jWUdICTdgc3Ibxmr8nAJiiLHwQBY0UI0XZcEMaFKaA=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240125205218-1f4bbc51befe h1:bQnxqljG/wqi4NTXu2+DJ3n7APcEA882QZ1JvhQAq9o=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240125205218-1f4bbc51befe/go.mod h1:PAREbraiVEVGVdTZsVWjSbbTtSyGbAgIIvni8a8CD5s=

Choose a reason for hiding this comment

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

I would make sure protocol team signs off on these version changes

go.mod Outdated
google.golang.org/protobuf v1.31.0
google.golang.org/genproto/googleapis/api v0.0.0-20240125205218-1f4bbc51befe
google.golang.org/grpc v1.61.0
google.golang.org/protobuf v1.33.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to bump protobuf version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, good question. I was not specifically looking to bump. But with ignite tools installed, I think this was done automatically on generation step. I can roll this back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yzang2019 addressed this

Then, to generate the code, run the following command:

```bash
ignite generate proto-go
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nicee!

dssei added a commit to sei-protocol/sei-cosmos that referenced this pull request Mar 19, 2024
Reverts #457 in favor of
sei-protocol/sei-chain#1444 as tokenfactory is
more appropriate module for the endpoint
@dssei dssei merged commit e1c2099 into main Mar 19, 2024
36 checks passed
@dssei dssei deleted the SEI-6857_add_tokenfcatory_denom_metadata_endpoint branch March 19, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants