Conversation
|
|
||
| // abci API | ||
| "abci_query": rpc.NewRPCFunc(ABCIQuery, "path,data,prove"), | ||
| "abci_query": rpc.NewRPCFunc(ABCIQuery, "path,data,height,prove"), |
There was a problem hiding this comment.
this is definitely the simplest approach, but I believe @cloudhead was arguing to remove the bool from here and use a separate abci_query_trusted method for prove = false, so the default is it's always true. Would that be better ? It might be nice if the options struct was an arg here that could go in through the jsonrpc, but itd be a total pain to call it using the URI - maybe we just shouldn't worry too much about that usecase, I kind of see us eventually deprecating the URI interface eventually due to issues like this
There was a problem hiding this comment.
be a total pain to call it using the URI
yeah.
As I understood, @ethanfrey and @cloudhead don't care as long as the HttpClient (or LocalClient) defaults to prove=true. And that's what I did: our both clients have this interface
ABCIQuery(path string, data []byte) { return ABCIQueryWithOptions(path, data, abci.DefaultQueryOptions) }
ABCIQueryWithOptions(path string, data []byte, opts abci.QueryOptions) { ... }
Or this is counterintuitive? I mean when the rpc core method defaults to prove=false, but the official client defaults to prove=true?
There was a problem hiding this comment.
I was more concerned with the Go API - but I do think they should all default to prove=true/trusted=false
There was a problem hiding this comment.
"yeah, it will be much easier if we rename prove to trusted"
rpc/client/types.go
Outdated
| // DefaultABCIQueryOptions are latest height (0) and prove equal to true. | ||
| func DefaultABCIQueryOptions() ABCIQueryOptions { | ||
| return ABCIQueryOptions{Height: 0, Prove: true} | ||
| } |
There was a problem hiding this comment.
It would be nice if this was a var instead of a function.
rpc/client/types.go
Outdated
| // than the DefaultABCIQueryOptions. | ||
| type ABCIQueryOptions struct { | ||
| Height uint64 | ||
| Prove bool |
There was a problem hiding this comment.
Since we're moving to Prove: true as the default, maybe switching this to Trusted bool, where Trusted == !Prove would be better, so that an empty ABCIQueryOptions{} would have the expected defaults.
rpc/core/abci.go
Outdated
| // | height | uint64 | 0 | false | Height (0 means latest) | | ||
| // | prove | bool | false | false | Include a proof of the data inclusion | | ||
| func ABCIQuery(path string, data data.Bytes, prove bool) (*ctypes.ResultABCIQuery, error) { | ||
| func ABCIQuery(path string, data data.Bytes, height uint64, prove bool) (*ctypes.ResultABCIQuery, error) { |
There was a problem hiding this comment.
Can this method have a similar signature to the other one, with ABCIQueryOptions?
There was a problem hiding this comment.
will be very hard to do
|
Good! |
Codecov Report
@@ Coverage Diff @@
## develop #734 +/- ##
===========================================
- Coverage 57.4% 57.09% -0.32%
===========================================
Files 82 82
Lines 8422 8434 +12
===========================================
- Hits 4835 4815 -20
- Misses 3171 3197 +26
- Partials 416 422 +6 |
rpc/client/types.go
Outdated
| Trusted bool | ||
| } | ||
|
|
||
| // DefaultABCIQueryOptions are latest height (0) and prove equal to true. |
There was a problem hiding this comment.
update comment - "trusted equal to false (which will result in a proof being returned)"
45a5af9 to
7518c4a
Compare
See #482