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

Feat/clients #145

Merged
merged 22 commits into from
Jul 23, 2022
Merged

Feat/clients #145

merged 22 commits into from
Jul 23, 2022

Conversation

pyramation
Copy link
Collaborator

No description provided.

return promise.then(data => QueryModuleAccountsResponse.decode(new _m0.Reader(data)));
}

Bech32Prefix(request: Bech32PrefixRequest): Promise<Bech32PrefixResponse> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@assafmo I noticed that the version in cosmjs didn't' have Bech32Prefix and AddressBytesToString — I'm guessing this is still correct. Does it look ok to you?

Copy link

Choose a reason for hiding this comment

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

These queries are new in cosmos-sdk v0.46: https://github.com/cosmos/cosmos-sdk/blob/5019459b1b2028119c6ca1d80714caa7858c2076/proto/cosmos/auth/v1beta1/query.proto#L37-L64

We should probably have a registry of protobuf derived ts definitions for every tagged version of every chain, tendermint, ibc-go, cosmos-sdk, liquidity-staking-module, etc.

Copy link

Choose a reason for hiding this comment

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

Then decouple the client code from these definitions. Have the client devs import the definitions of the chain that they're targeting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great catch! ok so this is correct but it's for the 0.46.

sounds like a good plan to support all the tagged versions. Now you got me thinking... in future we may have a situation where an app wants to use two chains, and one uses 0.45 and the other 0.46 and we need to support both in the same process.

Copy link
Collaborator Author

@pyramation pyramation Jul 22, 2022

Choose a reason for hiding this comment

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

an example use case: keplr governance UI and a user needs to vote. One chain uses 0.45 and another 0.46, my guess is, they'd need to have both cosmos versions.

I will continue to ponder the best way to support this. I'm almost inclined to just require the dev to make two different folders manually, although I can imagine a neat way to seamlessly allow the transpiler to bundle all the versions.

@pyramation pyramation marked this pull request as ready for review July 23, 2022 00:02
@pyramation pyramation merged commit ae660b5 into main Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants