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

[Clients] embed ParamsQuerier into SharedQueryClient #1000

Open
wants to merge 7 commits into
base: issues/543/params/query-client
Choose a base branch
from

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Dec 11, 2024

Summary

Embed the ParamsQuerier interface into SharedQueryClient and update both off- and on-chain implementations; i.e. sharedQuerier and sharedKeeperQueryClient, respectively.

---
title: Legend
---

classDiagram-v2

class GenericInterface__T__any {
    <<interface>>
    GenericMethod() T
}

class Implementer {
    ExportedField FieldType
    unexportedField FieldType
}

Implementer --|> GenericInterface__T__any: implements

class Embedder__T__any {
    <<interface>>
    GenericInterface[T]
}

Embedder__T__any ..|> GenericInterface__T__any: embeds

Implementer --* FieldType: composes*
Implementer --o FieldType: aggregates*

Loading

*Aggregation implies that the compose component instance DOES have an independent existence from its composition, composition DOES NOT.

---
title: Shared Querier Integration (off-chain)
---

classDiagram-v2

class ParamsQuerier__P__sdk_Msg {
    <<interface>>
    GetParams(ctx context.Context) (params P, err error)
    GetParamsAtHeight(ctx context.Context, height int64) (params P, err error)
}


class cachedParamsQuerier__P__sdk_Msg__Q__paramsQuerier {
    queryClient Q__paramsQuerier[P]
    paramsCache HistoricalQueryCache[P]
    ...
}
cachedParamsQuerier__P__sdk_Msg__Q__paramsQuerier --|> ParamsQuerier__P__sdk_Msg

class HistoricalQueryCache__T__any {
    <<interface>>
    QueryCache__T__any  // omitted from this diagram
    GetAtHeight(key string, heigt int64) (value T, err error)
    SetAtHeight(key string, value T, heigt int64) (err error)
}

cachedParamsQuerier__P__sdk_Msg__Q__paramsQuerier --* HistoricalQueryCache__T__any

class SharedQuerier {
    <<interface>>
    ParamsQuerier__P__sdk_Msg
}
SharedQuerier ..|> ParamsQuerier__P__sdk_Msg
sharedQuerier --|> SharedQuerier

class sharedQuerier {
    ParamsQuerier__P__sdk_Msg
}
sharedQuerier --* cachedParamsQuerier__P__sdk_Msg__Q__paramsQuerier
Loading
---
title: Shared Querier Integration (on-chain)
---

classDiagram-v2

class ParamsQuerier__P__sdk_Msg {
    <<interface>>
    GetParams(ctx context.Context) (params P, err error)
    GetParamsAtHeight(ctx context.Context, height int64) (params P, err error)
}

class keeperParamsQuerier__P__any__K__paramsKeeperIface {
    keeper K

    GetParams(ctx context.Context) (params P, err error)
    GetParamsAtHeight(ctx context.Context, height int64) (params P, err error)  // NEVER called; blocked by #931
}

keeperParamsQuerier__P__any__K__paramsKeeperIface --|> ParamsQuerier__P__sdk_Msg

%% class paramsKeeperIface__P__any {
%%     GetParams(context.Context) P
%% }
%% keeperParamsQuerier__P__any__K__paramsKeeperIface --> paramsKeeperIface__P__any

class sharedkeeper.Keeper {
    GetParams(ctx context.Context) (params P, err error)
    %% GetParamsAtHeight(ctx context.Context, height int64) (params P, err error)
}

keeperParamsQuerier__P__any__K__paramsKeeperIface --o sharedkeeper.Keeper

class SharedQuerier {
    <<interface>>
    ParamsQuerier__P__sdk_Msg
}
SharedQuerier ..|> ParamsQuerier__P__sdk_Msg
sharedKeeperQueryClient --|> SharedQuerier

class sharedKeeperQueryClient {
    ParamsQuerier__P__sdk_Msg

    sharedKeeper sharedKeeper
    sessionKeeper SessionKeeper  // omitted from this diagram
}
sharedKeeperQueryClient --* keeperParamsQuerier__P__any__K__paramsKeeperIface
sharedKeeperQueryClient --o sharedkeeper.Keeper
Loading

Issue

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@bryanchriswhite bryanchriswhite added off-chain Off-chain business logic on-chain On-chain business logic code health Cleans up some code consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. labels Dec 11, 2024
@bryanchriswhite bryanchriswhite self-assigned this Dec 11, 2024
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params-querier/shared branch from a19f686 to 1e3af86 Compare December 11, 2024 16:28
@bryanchriswhite bryanchriswhite changed the base branch from issues/543/params-querier/session to issues/543/params/query-client December 11, 2024 16:40
@bryanchriswhite bryanchriswhite linked an issue Dec 11, 2024 that may be closed by this pull request
4 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params/query-client branch 2 times, most recently from 49183d7 to ba61d14 Compare December 12, 2024 14:12
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params-querier/shared branch from 678f795 to f72639f Compare December 12, 2024 14:21
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 1000)
Grafana network dashboard for devnet-issue-1000

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Dec 12, 2024
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params-querier/shared branch 3 times, most recently from 9397ffe to 03269f1 Compare December 12, 2024 15:17
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params-querier/shared branch from d52af81 to d630484 Compare December 13, 2024 14:31
@bryanchriswhite bryanchriswhite force-pushed the issues/543/params-querier/shared branch from fd61a91 to ebc0b2e Compare December 13, 2024 15:59
@bryanchriswhite bryanchriswhite marked this pull request as ready for review December 13, 2024 16:00
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

I can tell there's a lot more work to do but this is a great step forward :)

// shared module's params type. This is required to resolve generic type constraints.
type SharedQueryClient interface {
QueryClient
GetParams(context.Context) (*Params, error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GetParams(context.Context) (*Params, error)
GetParams(context.Context) (*Params, error)
// TODO_MAINNET(@bryanchriswhite, #931): GetParamsAtHeight(context.Context, uint64) (*Params, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not accurate. The point of this interface is only to adapt from:

Params(context.Context, *QueryParamsRequest, ...) (*QueryParamsResponse, error)

to:

GetParams(context.Context) (*Params, error)

This interface is used as a generic type parameter to the NewCachedParamsQuerier() constructor which is what returns the object that defines GetParamsAtHeight().

Copy link
Member

Choose a reason for hiding this comment

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

Can you #PUC so this reasoning is clear to others please?

x/shared/keeper/params_query_client.go Show resolved Hide resolved
x/shared/keeper/params_query_client.go Outdated Show resolved Hide resolved
x/shared/keeper/params_query_client.go Outdated Show resolved Hide resolved
x/shared/keeper/params_query_client.go Outdated Show resolved Hide resolved
x/shared/keeper/params_query_client.go Show resolved Hide resolved
@Olshansk Olshansk changed the title [Clients] embed ParamsQuerier into SharedQueryClient [MERGE AFTER BASE IS MAIN][Clients] embed ParamsQuerier into SharedQueryClient Dec 14, 2024
@bryanchriswhite bryanchriswhite changed the title [MERGE AFTER BASE IS MAIN][Clients] embed ParamsQuerier into SharedQueryClient [Clients] embed ParamsQuerier into SharedQueryClient Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. devnet devnet-test-e2e off-chain Off-chain business logic on-chain On-chain business logic push-image CI related - pushes images to ghcr.io
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[Off-Chain] ModuleParamsClient & Historical Params
2 participants