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

[Bug]: LCD queries with {denom} don't work with slashes #18917

Closed
1 task done
jcompagni10 opened this issue Dec 29, 2023 · 9 comments · Fixed by #18956
Closed
1 task done

[Bug]: LCD queries with {denom} don't work with slashes #18917

jcompagni10 opened this issue Dec 29, 2023 · 9 comments · Fixed by #18956
Labels

Comments

@jcompagni10
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

I cannot find a way to use an ibc denom as part of the path for bank endpoints with a {denom} argument when making an HTTP request.
ie. cosmos/bank/v1beta1/denom_owners/{denom} or cosmos/bank/v1beta1/denoms_metadata/{denom}

cosmos/bank/v1beta1/denom_owners/ibc/FDEA8DF3742C0148CB1E04B2996823076AD960566AC20DFB8B0BA583B4BC0C56 returns a 501
as does cosmos/bank/v1beta1/denom_owners/ibc%2FFDEA8DF3742C0148CB1E04B2996823076AD960566AC20DFB8B0BA583B4BC0C56 (percent encoded)

I have tried a number of other permutations for encoding and cannot get it to work.
Is there a workaround here? Or is the routing simply unable to accept a "/" in the URL string.

Cosmos SDK Version

0.47

How to reproduce?

curl https://lcd.osmosis.zone/cosmos/bank/v1beta1/denom_owners/ibc/FDEA8DF3742C0148CB1E04B2996823076AD960566AC20DFB8B0BA583B4BC0C56

@facundomedica
Copy link
Member

facundomedica commented Dec 29, 2023

Hi, please take a look at #17493 , seems that this is not an issue for versions later than v0.46 (Osmosis might haven't backported the specific fix to their fork). Although for denom_owners it is still an issue

@jcompagni10
Copy link
Author

I am trying to get this to work on neutron running v0.47.6 and still having the same problem.

https://rest-kralum.neutron-1.neutron.org/cosmos/bank/v1beta1/denom_owners/ibc%2F00230C58F1540708067850B3362C8A67A273FC52F0C7305B802D7255495C86FD

@jcompagni10
Copy link
Author

Also, I know some of the endpoints now allow for a query param to supply {denom}, but I am only asking about the ability to supply the denom as a path parameter.

@tac0turtle
Copy link
Member

upgrading to grpc-gateway v2 fixes this. This is being worked on here #18872.

@tac0turtle tac0turtle moved this from 👀 To Do to ✍ In Progress in Cosmos-SDK Dec 30, 2023
@puneet2019
Copy link
Contributor

^ facing the same. can we do DenomOwners2, without the {denom} in URL? so it takes it as url?denom=ibc/xxx
I believe everything remains the same.
in protos

 rpc DenomOwners2(QueryDenomOwnersRequest) returns (QueryDenomOwnersResponse) {
    option (cosmos.query.v1.module_query_safe) = true;
    option (google.api.http).get               = "/cosmos/bank/v1beta1/denom_owners";
  }

in grpc querier

func (k BaseKeeper) DenomOwners2(
	goCtx context.Context,
	req *types.QueryDenomOwnersRequest,
) (*types.QueryDenomOwnersResponse, error) {
	return k.DenomOwners(goCtx, req)
}

@puneet2019
Copy link
Contributor

^ Looking at the grpc-gateway v2, i believe it can be backported. so no need of hacks.

@alexanderbez
Copy link
Contributor

According to @tac0turtle, we can't simply bump the grpcgateway dep to v2, as we have downstream deps that would break clients if we made the update. I'm not sure of the exact specifics, so perhaps @tac0turtle can chime in.

@tac0turtle
Copy link
Member

@puneet2019 @jcompagni10 are you guys using the rest endpoints in the browser or?

@puneet2019
Copy link
Contributor

puneet2019 commented Jan 5, 2024

@tac0turtle curl, browser both..

I had to get some data for airdrop users, some nodes didn't allow archival queries via grpc ( it works there ).. so had to fallback to rest ( do not usually use it ). but I got around it after grpc started working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants