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/use Comet Client #503

Merged
merged 20 commits into from
Dec 12, 2023
Merged

Conversation

hoangdv2429
Copy link
Contributor

Close #469

This PR will update the use of tendermint34Client (deprecated type) with cometClient, type of client will be dynamically chosen on time of creation.

Only the function used to create the client is different. Everything else remain backward compatible

@hoangdv2429
Copy link
Contributor Author

@Zetazzz Can you help me with the e2e test sir ? I'm pretty sure we already have connectComet at the new version.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Nov 1, 2023

@Zetazzz Can you help me with the e2e test sir ? I'm pretty sure we already have connectComet at the new version.

Hi, great work, Thank you very much!

Ok, I'll take a look on that

@Zetazzz
Copy link
Collaborator

Zetazzz commented Nov 6, 2023

@Zetazzz Can you help me with the e2e test sir ? I'm pretty sure we already have connectComet at the new version.

Hi, I've checked the function 'connectComet' has not been published in the latest version of @cosmjs/tendermint-rpc yet, so e2e can't build without the function.

So let's just wait for it.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Nov 6, 2023

And could you please add a flag 'rpcClients.useConnectComet' to control how users generate with the function or not. I think in this way, we let users to choose.

Thank you very much!

@hoangdv2429
Copy link
Contributor Author

I will add it soon. Thank you for reviewing.

@hoangdv2429
Copy link
Contributor Author

@Zetazzz Hii, sorry to ping you at this hour. I’m not sure but there is a lot of changes to the fixtures when I run yarn test:watch from the PR that I’m working on and those changes doesn’t relate to the changes I make. Can you have a look ? Thank you!

@Zetazzz
Copy link
Collaborator

Zetazzz commented Nov 13, 2023

@Zetazzz Hii, sorry to ping you at this hour. I’m not sure but there is a lot of changes to the fixtures when I run yarn test:watch from the PR that I’m working on and those changes doesn’t relate to the changes I make. Can you have a look ? Thank you!

Hi, no problem at all. And there're some changes posted recently, so please try to re-build(yarn build) before you run test. if that still happen, you can check in those into this PR and let's figure out.(please make sure to backup if it's necessary)

And if things in fixtures are conflict, feel free to ignore, delete and then re-gen them with your merged logic code.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Dec 7, 2023

Hi, @hoangdv2429

"@cosmjs/tendermint-rpc": "^0.32.1" seems ready.

So I guess we can proceed this PR now.

Please use a flag 'rpcClients.useConnectComet' to control whether we use connectComet or the old way.
And you can use v-next/telescope-v4 to test the flag and generated code.

Thank you very much!

@hoangdv2429
Copy link
Contributor Author

Screenshot 2023-12-08 at 02 26 08 I'm getting this weird error when running `yarn` or `yarn build`. I think the import path are misalign between starship and telescope. Have you ever encountered this before ?

@hoangdv2429
Copy link
Contributor Author

cc @Zetazzz thank you!

@Zetazzz
Copy link
Collaborator

Zetazzz commented Dec 7, 2023

Screenshot 2023-12-08 at 02 26 08 I'm getting this weird error when running yarn or yarn build. I think the import path are misalign between starship and telescope. Have you ever encountered this before ?

are those versions of cosmjs the same in telescope and starship folder?

@hoangdv2429
Copy link
Contributor Author

I think this dependency only exists in starship.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Dec 7, 2023

I think this dependency only exists in starship.

I see, let me take a look

@Zetazzz
Copy link
Collaborator

Zetazzz commented Dec 7, 2023

I think this dependency only exists in starship.

There're deps between cosmjs submodules, so I guess we should make all these the latest version:

"@cosmjs/stargate": "0.29.3",
"@cosmjs/tendermint-rpc": "^0.31.2",

@hoangdv2429
Copy link
Contributor Author

Thank you I will be checking on it tomorrow. That's probably the reason 🙏

Copy link

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Jupp, this is what you need

let functionStatements;
let awaitClientCreation;

if (newClientType) {

Choose a reason for hiding this comment

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

You should really need this case distinction long term. The first case should work everywhere. However, feel free to keep it for a trial-period.

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 discussed with @Zetazzz and he wants to make it into an option instead of default behavior. wdyt @Zetazzz ?

packages/starship/package.json Outdated Show resolved Hide resolved
packages/telescope/src/helpers/external.ts Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
@hoangdv2429
Copy link
Contributor Author

@webmaster128 @Zetazzz thank you sirs. I'll update it.

@hoangdv2429
Copy link
Contributor Author

Need to make another issue for upgrading all the deps.

This reverts commit 557cf39.
@hoangdv2429 hoangdv2429 merged commit 44144f1 into main Dec 12, 2023
2 checks passed
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.

need to make sure tendermint37 client is also working
3 participants