feat(bank): Add secondary query for bank metadata #16852

mattverse · 2023-07-06T07:04:26Z

Description

Current bank metadata query takes in denom field as query path, thus does not support querying any denom with "/". (Ex tokenfactory/~") cannot be queried through current query.

This PR adds secondary query for denom meta data, where here we take in query as query string, thus also supports query as above.

The reason this query is designed as a secondary query is because last time we have made similar change to the supply query, it was API breaking thus effected(broke) lots of products or contracts having dependency on the API. This way of creating a secondary query would not break any other dependency.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

CHANGELOG.md

julienrbrt · 2023-07-06T10:45:29Z

api/cosmos/bank/module/v1/module.pulsar.go

how come this has changed?

This I do not know, probably something to do with protoc or how we generate pulsar files, do you want me to revert this file for now?

Just running make proto-gen should suffice.

I have used the same command for proto gen 🤔 I'm wondering if we check docker image versions for protogen and if that might be the cause for this?

julienrbrt

utACK

alexanderbez · 2023-07-06T16:05:25Z

x/bank/keeper/grpc_query.go

@@ -225,6 +225,28 @@ func (k BaseKeeper) DenomMetadata(c context.Context, req *types.QueryDenomMetada
 	}, nil
 }

+// DenomMetadataByQueryString is identical to DenomMetadata query, but receives request via query string.
+func (k BaseKeeper) DenomMetadataByQueryString(c context.Context, req *types.QueryDenomMetadataByQueryStringRequest) (*types.QueryDenomMetadataByQueryStringResponse, error) {


Am I missine something but isn't this the same as DenomMetadata?

If the issue is just the fact that we MUST provide a denom in the URL path, then I would avoid creating a duplicate query handler -- we can at least extract out the handler code into a single method.

In addition, is there no annotation or something that we can add to the existing query to allow an empty query argument?

we can at least extract out the handler code into a single method.

@alexanderbez We need to suffice the BaseKeeper Interface, thus need to have this method with this specific request field and response field. What I can do is extract the duplicated method between DenomMetadata and DenomMetadataByQueryString` to reduce code duplication.

Yes, the code within the query handlers are identical I believe, so that should be kept DRY 👍

On the second look, the way it is right now seems as far as we could go for having non-duplicated code.
We use the same abstracted keeper method, namely k.GetDenomMetaData whilst the remaining part is simply handling the request and response itself

alexanderbez

ACK

alexanderbez · 2023-07-10T15:03:15Z

proto/cosmos/bank/v1beta1/query.proto

+  rpc DenomMetadataByQueryString(QueryDenomMetadataByQueryStringRequest)
+      returns (QueryDenomMetadataByQueryStringResponse) {
+    option (cosmos.query.v1.module_query_safe) = true;
+    option (google.api.http).get               = "/cosmos/bank/v1beta1/denoms_metadata_by_query_string";


Suggested change

option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata_by_query_string";

option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata_by_query";

IMO, query string seems like the correct term to use, is there a reason for suggestion above?

Yes, a few items:

It makes an already verbose query even more verbose without adding context

The fact that denom is a string is already available based on the proto schema

Generally, you dont wan't to annotate a query's path (e.g. /users/by_name vs /users/by_name_string), the latter doesn't give you anything as it's superfluous IMO

Oh I was referring to query string as a single term here though (https://en.wikipedia.org/wiki/Query_string)

I'm happy with it either way, but just wanted to make things clear :)

Not a blocker for me, just my two cents :p

we can merge

julienrbrt · 2023-07-12T09:11:49Z

If the naming is okay, can you run make proto-swagger-gen?

julienrbrt · 2023-07-26T08:27:18Z

Gentle ping @mattverse

mattverse · 2023-07-27T14:58:03Z

@julienrbrt Done!

(cherry picked from commit c295832) # Conflicts: # CHANGELOG.md # x/bank/keeper/grpc_query.go

…17168) Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

mattverse added 4 commits July 5, 2023 19:10

WIP

1f86b12

Finish implementation

a0b9e18

Remove unnecessary proto parsing

eb8187f

Remove unnecessary proto parsing

c6f575e

mattverse requested a review from a team as a code owner July 6, 2023 07:04

github-actions bot added the C:x/bank label Jul 6, 2023

github-prbot requested review from a team, aaronc and atheeshp and removed request for a team July 6, 2023 07:05

Add changelog

e98f599

julienrbrt changed the title ~~!feat: Add secondary query for bank metadata (non-api breaking)~~ feat(bank): Add secondary query for bank metadata Jul 6, 2023

Fix lint

69b3ed1

github-actions bot added the C:x/gov label Jul 6, 2023

Merge branch 'main' into mattverse/banks

d90b997

julienrbrt reviewed Jul 6, 2023

View reviewed changes

Update changelog

2f1cb8c

julienrbrt approved these changes Jul 6, 2023

View reviewed changes

alexanderbez reviewed Jul 6, 2023

View reviewed changes

alexanderbez approved these changes Jul 10, 2023

View reviewed changes

Merge branch 'main' into mattverse/banks

2816b78

Swagger gen

d4bdfa4

github-actions bot added the C:CLI label Jul 27, 2023

julienrbrt enabled auto-merge July 27, 2023 15:08

julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jul 27, 2023

Merge branch 'main' into mattverse/banks

48b053f

julienrbrt added this pull request to the merge queue Jul 27, 2023

Merged via the queue into cosmos:main with commit c295832 Jul 27, 2023

mergify bot pushed a commit that referenced this pull request Jul 27, 2023

feat(bank): Add secondary query for bank metadata (#16852)

87aea98

(cherry picked from commit c295832) # Conflicts: # CHANGELOG.md # x/bank/keeper/grpc_query.go

mergify bot mentioned this pull request Jul 27, 2023

feat(bank): Add secondary query for bank metadata (backport #16852) #17168

Merged

julienrbrt added a commit that referenced this pull request Jul 27, 2023

feat(bank): Add secondary query for bank metadata (backport #16852) (#…

d200d42

…17168) Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

faddat mentioned this pull request Nov 8, 2024

faddat/many times #22476

Closed

12 tasks

This was referenced Nov 18, 2024

[Bug]: rpc DenomMetadata can't support slash #22542

Closed

feat(x/bank): Allow value with slashes in DenomMetadata #22543

Merged

mergify bot mentioned this pull request Nov 25, 2024

feat(x/bank): Allow value with slashes in DenomMetadata (backport #22543) #22640

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bank): Add secondary query for bank metadata #16852

feat(bank): Add secondary query for bank metadata #16852

mattverse commented Jul 6, 2023 •

edited

Loading

julienrbrt Jul 6, 2023

mattverse Jul 6, 2023

julienrbrt Jul 6, 2023

mattverse Jul 6, 2023 •

edited

Loading

julienrbrt left a comment

alexanderbez Jul 6, 2023

alexanderbez Jul 6, 2023

mattverse Jul 7, 2023

alexanderbez Jul 7, 2023

mattverse Jul 10, 2023

alexanderbez left a comment

alexanderbez Jul 10, 2023

mattverse Jul 11, 2023

alexanderbez Jul 11, 2023

mattverse Jul 11, 2023

mattverse Jul 11, 2023

alexanderbez Jul 11, 2023

julienrbrt commented Jul 12, 2023

julienrbrt commented Jul 26, 2023

mattverse commented Jul 27, 2023

	option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata_by_query_string";
	option (google.api.http).get = "/cosmos/bank/v1beta1/denoms_metadata_by_query";

feat(bank): Add secondary query for bank metadata #16852

feat(bank): Add secondary query for bank metadata #16852

Conversation

mattverse commented Jul 6, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt commented Jul 12, 2023

julienrbrt commented Jul 26, 2023

mattverse commented Jul 27, 2023

mattverse commented Jul 6, 2023 •

edited

Loading

mattverse Jul 6, 2023 •

edited

Loading