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

Allow for unsafe synchronous creation of Tendermint34Client and Tendermint35Client #1358

Closed
garrettparris opened this issue Dec 29, 2022 · 5 comments · Fixed by #1380
Closed

Comments

@garrettparris
Copy link

Currently the only way to use init the query client is asynchronously. When loading up a webpage relying on this client, all queries must wait on the detectVersion async function to complete and only then can the app use the query client. I suggest adding an unsafe creation method to the TendermintClient classes that skips this async detectVersion function.

@garrettparris
Copy link
Author

garrettparris commented Dec 29, 2022

#1359
Relevant PR

@webmaster128
Copy link
Member

The query we are doing there is very questionable anyways. Would be good to understand if it is still needed and why.

What's your primary motivation for the proposed change?

  • Getting rid of the async constructions?
  • Avoid having to wait for the network request?

I'd like to get this integrated in the main constructors and solve your issue at the same time. Adding more APIs is probably not necessary here.

@garrettparris
Copy link
Author

on the existing create function there is the following comment

  public static async create(rpcClient: RpcClient): Promise<Tendermint34Client> {
    // For some very strange reason I don't understand, tests start to fail on some systems
    // (our CI) when skipping the status call before doing other queries. Sleeping a little
    // while did not help. Thus we query the version as a way to say "hi" to the backend,
    // even in cases where we don't use the result.

In my opinion, health checking should be something the developer adds and not something baked into the creation of the class. However, the above comment is why I opted to add its own creation method.

My primary motivations are to remove the async constructions and avoid waiting for the network request. Doing so will allow clients to query over grpc on load time rather than waiting for this async to resolve.

@garrettparris
Copy link
Author

@webmaster128 any thoughts on this?

@webmaster128
Copy link
Member

I tried removing the extra query again but it causes connection problems again.

In #1380 you see and approach where the version query is moves from .create to .connect. You can use create directly then. It's still async but resolves immediately without a network request. Would that solve your issue?

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 a pull request may close this issue.

2 participants