Conversation
|
Example usage (end-user version) is in the tests: https://github.com/tendermint/tendermint/blob/tmsp_proof/rpc/test/client_test.go#L174-L209 Is there somewhere else I should expose this? |
rpc/core/routes.go
Outdated
| } | ||
|
|
||
| func TMSPProofResult(key []byte, height int64) (ctypes.TMResult, error) { | ||
| fmt.Println("ONE") |
There was a problem hiding this comment.
debug statement. removed and gone from git history
Current coverage is 49.20% (diff: 0.00%)@@ unstable #356 diff @@
==========================================
Files 36 36
Lines 4457 4447 -10
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 2226 2188 -38
- Misses 2018 2043 +25
- Partials 213 216 +3
|
|
Why are we creating a new tmsp message? Why not just leave this to the Query? |
|
Query just passes in an arbitrary byte struct meant to be deciphered by the client in any way they see fit, and returns what they see fit (often json in the examples). Proof requires that the argument is a key and the result a merkle proof in hex-encoded binary form. Sure, we could overload them, using the first byte of the argument to query to determine proof or query, but I don't see that makes the code better. It also adds more hidden requirements to the people implementing tmsp applications (meaning rather than getting a compile time error, suddenly queries would not work as expected). We could also overload AppendTx and CheckTx by using the first byte as 0x0 or 0x1 to distinguish between them, but that wasn't done. I figured actions with different semantics should use different messages. Also, proof actually accepts two arguments, where query accepts one (the second being a currently unsupported blockheight, which is essential in Jae's opinion - and mine - to efficiently implementing IBC). But then again, this was done quick to get something for Jae to demo. We can have a longer discussion on it and if you and Jae agree on another implementation, I'll accept your decision. |
|
Ok great. That makes sense. I think I'm convinced. Is there anything else you feel is missing from TMSP so far? I'd rather try and get this merged in a non-breaking way as an optional feature for apps in 0.8.1. We can make it required in 0.9 |
|
Personally, I have a bit different idea of the architecture of tmsp, since that long thread with wolfspod a few months ago. I talked with Jae about it before implementing this, but he wanted something that would work now, without rethinking the design. Fair enough, but when there is time to discuss that, I think it would be very important. You can accept or ignore the rest of this comment. I think it needs a bigger space to discuss than a github issue in any case. < rant > In that respect, I would remove I would also remove tmsp_query, tmsp_info, and tmsp_proof from the tendermin core rpc. (And probably the unsafe* methods as well) Tendermint core is a consensus engine and handles all P2P between nodes, and provides quick BFT consensus on the global ordering of all action. That is amazingly powerful and it's mission should not grow beyond that. A TMSP app can produce it's own web server, which handles all queries in whatever form it wishes without these restrictions from the tendermint rpc-tmsp communication layer. And without adding that load to the core. What if I want to provide a websocket listener when a key changes? How would I model that over tendermint rpc - tmsp and back? What if I want to configure my own CORS and allow gzip compression on large responses. Oh, and maybe throttle requests per IP.... All major languages have tooling to easily add these features to a web server. Let the app developers handle it. It is to many decisions and would take to much time to find the proper solution to everyone in tendermint core. The main entry from a light client into any application built on tendermint should be some API chosen by the app developers. Independent of the tendermint API. This allows MUCH faster iteration and customization. Tendermint core should just focus on being a consensus engine and generally hidden from the end user (like a cluster of etc nodes must communicate, possibly over geographically distributed areas, but are not exposed to the end users). Rather than trying to do everything for the developers though the RPC, we could build some tools to make it easy to setup and then customize your API in a few languages and encourage the community to extend them. This would allow app devs from various languages to make the interfaces they need to work, without the barrier to entry of understanding and modifying tendermint core. Hope that wasn't too much of an earful..... |
|
Strongly agree with the sentiment. Those rpc commands are useful for debugging. Looking forward to all the upcoming changes we'll be rolling out together. I think we should maybe start a design doc for TMSP and work towards something we can hopefully think about finalizing in 0.9, as we move towards 1.0 |
|
To much renaming craziness. I have decided to kill this PR in favor of #372 |
Added rpc endpoints to expose new TMSP ProofSync endpoint. To address #355
This (all of these) PR was done in a sprint, there are probably some chunks of code needing cleanup, please give feedback. Also on the tmsp reorg.