Improve wasm contract queries #22
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 56.48% 60.31% +3.82%
==========================================
Files 11 11
Lines 979 1023 +44
==========================================
+ Hits 553 617 +64
+ Misses 375 351 -24
- Partials 51 55 +4
Continue to review full report at Codecov.
|
alpe
force-pushed
the
improve_contract_queries_12
branch
from
249c7fe to
9156f29
Compare
There was a problem hiding this comment.
Very nice.
I think a little more polish on the cli package, and it is good to merge.
Once you feel it is ready, please compile this and try out the last section of the tutorial with it, even using the standard escrow contract. To make sure at least raw and all state commands work in the cli (I don't think there are any automatic tests for that).
The smart queries, I will test via cli after merging in 0.6 support
There was a problem hiding this comment.
Very nice.
I have two comments on TestQueryContractState that I am unsure about the test cases. I would suggest adjusting them, and adjusting the code as needed to fit.
More "NotFound" errors, and no result on miss. But maybe you can explain why it is better how you coded it.
Rest of the code looks wonderful (this does to, I just think the logic is a bit off)
| for iter := keeper.GetContractState(ctx, contractAddr); iter.Valid(); iter.Next() { | ||
| state = append(state, types.Model{ | ||
| resultData = append(resultData, types.Model{ |
| srcPath: []string{QueryGetContractState, anyAddr.String(), QueryMethodContractStateAll}, | ||
| expModelLen: 0, | ||
| }, | ||
| "query smart with unknown address": { |
There was a problem hiding this comment.
I would assume all the "query with unknown address" would return the same types.ErrNotFound("contract") message
There was a problem hiding this comment.
Agree ErrNotFound would be nicer but the prefix.Store concatenates the prefix and key for the parent store so that it results in just another unknown key.
I would need to do a prefix scan on the parent to detect the issue. The previous implementation for all had the same "problem"
| expModelLen: 1, | ||
| //expModelContains: []model{}, // stopping here as contract internals are not stable | ||
| }, | ||
| "query unknown raw key": { |
There was a problem hiding this comment.
Why doesn't this return an empty slice?
There was a problem hiding this comment.
This was the simplest implementation but not necessarily the best. I was under the assumption that the KV store allows nil values so this result make sense but it does not.
There was a problem hiding this comment.
Maybe you are right, but I would rather let len(0) = deleted. Even if not strictly true, it gives contracts a way to mark things as deleted
There was a problem hiding this comment.
I added a patch. And also returning non nil model slices.
|
Thanks for the cleanup. Looks great |
Resolves #12
In this PR are the 3 different query types implemented as defined in #12. This includes server side and cli code.
Example:
They all return:
[ { "key": "config", "val": "{\"arbiter\":\"cosmos1s5k2p7nct27jzjrzvqgwy785wug0js9vdmdr5s\",\"recipient\":\"cosmos1p35xmxjl8ysxdp699ff4kmca8duwcve2jupxe3\",\"source\":\"cosmos1laapuw0u4vx3jjyq8vtf3pmzj24h3eqdfunyuk\",\"end_height\":0,\"end_time\":0}" } ]For Admin Use: