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

fix!: update consumer chain queries #2246

Merged
merged 3 commits into from
Sep 11, 2024
Merged

fix!: update consumer chain queries #2246

merged 3 commits into from
Sep 11, 2024

Conversation

MSalopek
Copy link
Contributor

Add consumer_id to single chain response.

Add pagination for consumer chains.

@MSalopek MSalopek requested a review from a team as a code owner September 10, 2024 14:44
@github-actions github-actions bot added the C:x/provider Assigned automatically by the PR labeler label Sep 10, 2024
@@ -685,6 +685,11 @@ func ConsumerIdToPhaseKey(consumerId string) []byte {
return StringIdWithLenKey(mustGetKeyPrefix(ConsumerIdToPhaseKeyName), consumerId)
}

// ConsumerIdToPhaseKeyPrefix returns the key prefix used to iterate over all the consumer ids and their phases.
Copy link
Contributor

Choose a reason for hiding this comment

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

In line 685 (above) do return return StringIdWithLenKey(ConsumerIdToPhaseKeyPrefix(), consumerId) instead of return StringIdWithLenKey(mustGetKeyPrefix(ConsumerIdToPhaseKeyName), consumerId).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it

Copy link
Contributor

Choose a reason for hiding this comment

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

See @mpoke comment here:

mustGetKeyPrefix(ConsumerIdToRegistrationRecordKeyName). The idea is to have only one method calling mustGetKeyPrefix for a given key and for all these methods to be part of the UTs so that we make sure the panic in mustGetKeyPrefix is not triggered.

ConsumerMetadata metadata = 4 [ (gogoproto.nullable) = false ];
ConsumerInitializationParameters init_params = 5;
PowerShapingParameters power_shaping_params = 6;
string consumer_id = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

The consumer_id seems to be part of the QueryConsumerChainRequest. If so, what's the point of returning it as part the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we can validate on the FE that we got the chain_id for the consumer_id we requested. But, more importantly so that we don't have to keep extra context (e.g coming from URL params) about which chain we're dealing with.

E.g. previously we had to read the URL param for consumer_id for any operation we were doing:

forge/consumer-id/123

So we had to "remember" the 123. This way we look up an object and treat is as the source of truth and do all operations required by fetching the consumer_id from the returned object.

Does that make sense?

@@ -720,14 +722,19 @@ func TestQueryConsumerChains(t *testing.T) {
tc.setup(ctx, pk)
req := types.QueryConsumerChainsRequest{
Phase: tc.phase_filter,
Limit: tc.limit,
Pagination: &sdkquery.PageRequest{
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we test this, but would be nice to have a test that uses the PageResponse to construct the new PageRequest and verify that everything works as expected.

@mpoke
Copy link
Contributor

mpoke commented Sep 10, 2024

Fix gosec.

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

LGTM

@sainoe sainoe self-requested a review September 11, 2024 12:23
@mpoke mpoke merged commit db9176c into main Sep 11, 2024
13 of 16 checks passed
@mpoke mpoke deleted the masa/chains-pagination branch September 11, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/provider Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants