cosmjs
cosmjs copied to clipboard
cosmos
Add query*Block functions that report height
This change introduces parallel implementations of queryVerified and queryUnverified that return an object containing both the value and the height it was found at. This would allow us at Agoric to avoid a round trip through abciInfo to discover the current height before calling queryVerified.
(Please feel free to treat this as an issue-with-code.)
cc @webmaster128. Please let us know if this is a direction you’d entertain. Normally I’d open an issue first, but I think code speaks in this case.
Thank you. This makes sense overall. Just a few thoughts:
- It would be good to have a dedicated return type for both functions to not rely on tendermint-rpc details. Which fields of AbciQueryResponse do you need?
- I would go one step further and let
queryVerified/queryUnverifiedreturn the data you need instead of maintaining two more public methods. - Whats the reasoning behind the "Block" term? I don't understand it yet.
For 2. we need a migration path but we can break the API in 0.30. How urgent is the diff for you?
I’ve not reasoned hard about the Block suffix; please consider it a placeholder. Envelope,WithHeight, or any suitable name for a data + height struct should suffice. There’s no ideal name, of course, and you’re right that we could present the fatter result object under the same function name, except for the migration story.
If the notion is that replace the existing methods in the fullness of time, we could also greatly simplify the migration story if we just changed the name of the method to something equally sensible, like queryProven and queryUnproven. Then it’s straight-forward to deprecate queryVerified with the message “Please switch to using queryProven and extract the data field of the result. For queries with an unspecified height, the result will also include height”. Then in the next breaking-change version-bump release, remove the old methods.
I looked into this more now. I think one of the main API confusion comes from the naming "queryVerified"/"queryUnverified" which somehow imply a symmetry that does not exist. queryUnverified is an ABCI query that includes all sort of query paths like node info and reflection service. queryVerified on the other hand can only access storage and process the proofs. I'll break this down a bit and will ask for your review on the individual steps.
Step 1 is here: https://github.com/cosmos/cosmjs/pull/1328. @kriskowal could you check if the new method serves your needs?
@webmaster128 that works for our unverified case. Are you planning changes to support queryVerified returning the height it gets from queryRawProof?
Yeah, I do. Unfortunately I'm currently busy with CosmWasm.
This is more an API design challenge than anything else. But naming matters. I see step 2 as creating a something along the lines of queryStorageVerified(...) to make a clear differentiation between "query ABCI" and "query storage".
See https://github.com/cosmos/cosmjs/pull/1337 for the second part of this. I called the new function queryStoreVerified as it can only query a store, not any ABCI query. Also I added a note to the limitations: It verifies the responses based in unverified block headers. But changing that would be a massive project.
Reviews welcome.
Closing in favour of #1328 and #1337. Sorry for letting you wait so long. But now I'm quite happy with the API that explains much better what is actually queried. Thank you for your contribution and reviews!