Tags in DeliverTx response #130

melekes · 2017-11-09T21:53:49Z

No description provided.

ebuchman

Thanks for linting!

What's your thinking behind value_int ? Why not just have the string value ? If there are things we know we need numbers for, like gas, we should probably just build those in direct.

melekes · 2017-11-14T17:26:11Z

If there are things we know we need numbers for, like gas, we should probably just build those in direct.

my thinking was by declaring a type we're avoiding parsing errors. For example, tag "count_participants": 5. And the user wants to query "count_participants > 3". If value were a string ("5"), we'd have to parse it as Int each time and then compare against the query. If the value is an int, we're avoiding parsing (protobuf ensures it is an int64) and can also save it as int in the DB. But maybe this is not worth adding a new field.

message KVPair {
  string key = 1;
  string value_string = 2;
  int64 value_int = 3;
}

ebuchman · 2017-11-14T19:39:11Z

Interesting. I'm not sure if it's worth it. What does @ethanfrey think?

melekes · 2017-11-14T19:49:00Z

Originally this design was suggested by Jae.

melekes · 2017-11-15T22:40:33Z

message KVPair {
  enum Type { STRING = 1; INT = 2; }
  
  string Key = 1;

  Type type = 2;
  string ValueString = 3;
  int64 ValueInt = 4;
}

we can also add a type so we could switch in code ^^^ and later extend it with other types (like Date or Time). use case is when user wants to pass "" as a value

ethanfrey · 2017-11-16T08:28:04Z

I originally proposed a list of strings and only doing exact matches. thus it would match a specific key-value pair.

This concept was turned much more complex to the string-{string,int} model proposed in a meeting with anton, jae, and bucky. And was the last consensus in the design document: https://gist.github.com/melekes/131291e5144e31d34ac3ed3ab7ff2aca#gistcomment-2127313

AFAIK, the only use cases of the queries needed by the API are for exact matches, and if we are not using integer comparison (does anyone really need to filter all tx with a value over 40 atom regardless of address?), I am fine using only string equality (i can encode addresses as strings).

If we do support both int and strings and comparison operators in the indexer, however, it does make sense to expose that in the abci api.

Given the design space that was agreed upon, I approve this pr, but please use different result types here:
https://github.com/tendermint/abci/blob/feature/tags-in-deliver-tx/types/application.go#L16
and here:
https://github.com/tendermint/abci/blob/feature/tags-in-deliver-tx/types/application.go#L21

ethanfrey

I think we need to separate out DeliverTx and CheckTx, otherwise, the implementation looks good.

ethanfrey · 2017-11-16T08:30:18Z

types/result.go

@@ -102,6 +103,7 @@ func (r *ResponseCheckTx) Result() Result {
 		Code: r.Code,
 		Data: r.Data,
 		Log:  r.Log,
+		Tags: r.Tags,


There is no meaning to tags in checktx, as they should never be indexed. We should remove that from the api, as it would be misleading.

ethanfrey · 2017-11-16T08:32:43Z

types/result.go

@@ -12,6 +12,7 @@ type Result struct {
 	Code CodeType   `json:"code"`
 	Data data.Bytes `json:"data"`
 	Log  string     `json:"log"` // Can be non-deterministic
+	Tags []*KVPair  `json:"tags"`


I think we should start separating the result from DeliverTx and CheckTx.
DeliverTx needs tags, which CheckTx doesn't.
We will also need to add Gas and Fees (or Cost and Value or....) to CheckTx as per #57 and soon.

Given this, I already separated the DeliverTx and CheckTx results in the sdk. We should use different types in the abci.

melekes · 2017-11-16T16:36:07Z

I don't think it's too late to switch to Redis like subscriptions in that is what we want and it is enough for everyone.

ethanfrey · 2017-11-17T11:48:51Z

Added comments on slack, but they belong here:

We actually don't need any general purpose > < and strings are fine for subscriptions.
We only need to add a special case height int that supports < and > just for search, not subscriptions.

If we actually used redis as a backing store for pub sub (just as an example, we don't need to), sorted sets would satisfy the requirements of exactly one, predefined integer sort:

https://redis.io/topics/data-types#sorted-sets
https://redis.io/commands/zrangebyscore

This technique is explained in detail here (just skip the java code in the middle):

https://www.infoworld.com/article/3212768/database/how-to-use-redis-for-real-time-stream-processing.html?page=2

I am not saying we have to use redis as a backing store (although I think it might be nice to support it as an optional backend), but more illustrating that if we simplify the requirements to string channels and search by (exact channel, min height, max height), it becomes a much simpler problem to implement, as redis is a very simple k-v store.

I could also implement this sorted set query pattern on top of level db without much problem, and as anton has already implemented pubsub, that would cover the requirements.

ethanfrey · 2017-11-17T11:48:54Z

btw, my approach for handling this directly on leveldb, which supports iteration, would be to define keys as (channel string, block uint64, index uint32) tuples, using uncompressed bigendian for the uints so their binary representation has the same lexographical order as an integer comparison. And prefixing string with the length to avoid overlap in searches between strings like "foo" and "foobar" (or another approach that acheives the same).

You could then calculate byte representations for (channel, min block, 0) and (channel, max block, MAX_UINT32) and run a level db iterator over that space. Super easy and efficient (edited)

The purpose of adding tx index as a third parameter is to avoid collisions if there are multiple tx in the same block that match a channel

melekes · 2017-11-17T16:23:24Z

I like that idea, but the question is that enough for UI? https://allinbits.slack.com/archives/C564C4P1U/p1510910368000182?thread_ts=1510859626.000247&cid=C564C4P1U

add value_type field to KVPair

…spectively Commit now returns ResponseCommit

which was introduced in tendermint/abci#130

melekes · 2017-11-23T01:19:13Z

Addressed all the comments.

melekes · 2017-11-23T01:50:54Z

types/base_app.go

-func (app *BaseApplication) DeliverTx(tx []byte) Result {
-	return NewResultOK(nil, "")
+func (BaseApplication) DeliverTx(tx []byte) ResponseDeliverTx {
+	return ResponseDeliverTx{Code: CodeType_OK}


btw, why DeliverTx/CheckTx/Commit return OK, but Query returns empty ResponseQuery{}?

LOL good eye. Should be effectively the same since OK is the zero value, but we should be consistent.

which was introduced in tendermint/abci#130

See #130 (comment)

…Commit

ebuchman · 2017-11-27T17:53:41Z

types/result.go

@@ -6,114 +6,47 @@ import (
 	"github.com/tendermint/go-wire/data"
 )


Do we use the methods from this file anywhere?

yes, in Tendermint

ebuchman · 2017-11-27T17:53:50Z

types/result.go

 	}
 }

+// ResultQuery is a wrapper around ResponseQuery using data.Bytes instead of
+// raw byte slices.


Do we still need it?

melekes · 2017-11-27T22:03:31Z

Makefile

-	@ bash tests/test.sh
+
+test_race: 
+	@ find . -path ./vendor -prune -o -name "*.sock" -exec rm {} \;


this line is needed for test_integrations!

melekes · 2017-11-27T22:09:56Z

example/block_aware/block_aware_test.go

@@ -1,57 +0,0 @@
-package main


why remove this?

its not testing anything new. used to be when BlockAware was it's own interface. Now it's just part of Application.

melekes · 2017-11-27T22:14:14Z

CHANGELOG.md

+
+FEATURES:
+ - [types] added Tags field to ResponseDeliverTx
+ - [types] added Gas and Fee fields to ResponseCheckTx


want to add this to readme?

Yes thanks for reminder!

melekes · 2017-11-27T22:15:24Z

test.sh

+echo "==> Running integration tests (./tests)"
+find . -path ./vendor -prune -o -name "*.sock" -exec rm {} \;
+make install
+bash tests/test.sh


actually, we can call make test_integrations here. this will allows us to remove redundant find . -path ... line and make install line

dont we want test_integrations to run the shell scripts too tho ?

oh, i see. ok

melekes · 2017-11-27T22:32:11Z

ebuchman · 2017-11-27T22:42:02Z

BOOM

which was introduced in tendermint/abci#130

melekes force-pushed the feature/tags-in-deliver-tx branch from 36a19f2 to 6b47155 Compare November 9, 2017 22:18

melekes mentioned this pull request Nov 9, 2017

Indexing DeliverTx tags tendermint/tendermint#835

Merged

10 tasks

ebuchman approved these changes Nov 14, 2017

View reviewed changes

ethanfrey reviewed Nov 16, 2017

View reviewed changes

melekes mentioned this pull request Nov 17, 2017

Redis-like subscriptions tendermint/tendermint#874

Closed

melekes added 8 commits November 20, 2017 16:56

install protoc

480e4e4

add Dockerfile for development

87072d3

add proto section to .editorconfig

f07c300

add tags field to DeliverTx and CheckTx

bb0d7e9

reformat types.proto

33b5137

add docker commands to Makefile

76bd68f

add linter for proto files (Fixes #128)

8b71e47

no need for protoc in make all since we have types.pb.go generated

3cbf440

melekes force-pushed the feature/tags-in-deliver-tx branch from 6b47155 to 3cbf440 Compare November 20, 2017 23:02

melekes added 5 commits November 20, 2017 17:09

[make install_protoc] fix folder name

29c1cd0

add ldconfig cmd to Dockerfile

0239907

remove tags from CheckTx

fc7db13

add value_type field to KVPair

[dockerfile] install psmisc for tests

92801db

CheckTx and DeliverTx return ResponseCheckTx and ResponseDeliverTx re…

3a3d508

…spectively Commit now returns ResponseCommit

melekes force-pushed the feature/tags-in-deliver-tx branch from c92ced2 to 3a3d508 Compare November 21, 2017 02:37

melekes changed the title ~~WIP: tags~~ Tags in DeliverTx response Nov 21, 2017

melekes added a commit to tendermint/tendermint that referenced this pull request Nov 23, 2017

adapt Tendermint to new abci.Client interface

dd76519

which was introduced in tendermint/abci#130

melekes added 3 commits November 22, 2017 19:37

refactor code

0176a83

fix formatting of panicf function

03fafee

preserve behaviour of BaseApplication

9b30fab

melekes commented Nov 23, 2017

View reviewed changes

melekes added a commit to tendermint/tendermint that referenced this pull request Nov 24, 2017

adapt Tendermint to new abci.Client interface

2f31bca

which was introduced in tendermint/abci#130

melekes and others added 5 commits November 24, 2017 16:45

set Code to OK in ResponseQuery in BaseApplication

67d2a5f

See #130 (comment)

everything takes Request, returns Response; expect DeliverTx/CheckTx/…

c7f54fb

…Commit

fixup tests

fb612e5

types: add gas and fee fields to CheckTx

7dc5b74

minor fix

e08885e

ebuchman reviewed Nov 27, 2017

View reviewed changes

ebuchman added 4 commits November 27, 2017 20:19

update changelog

e9094fb

minor things

5a46675

linter

26d967a

types.pb.go isnt linted. use nolint :(

ab51bde

melekes commented Nov 27, 2017

View reviewed changes

ebuchman and others added 2 commits November 27, 2017 22:28

update README for CheckTx Gas and Fee

0981c17

move comment

a0bf6dc

ebuchman merged commit 460c045 into develop Nov 27, 2017

ebuchman deleted the feature/tags-in-deliver-tx branch November 27, 2017 22:42

melekes added a commit to tendermint/tendermint that referenced this pull request Nov 28, 2017

adapt Tendermint to new abci.Client interface

808a8d7

which was introduced in tendermint/abci#130

melekes added a commit to tendermint/tendermint that referenced this pull request Nov 29, 2017

adapt Tendermint to new abci.Client interface

f65e357

which was introduced in tendermint/abci#130

ethanfrey mentioned this pull request Jan 5, 2018

Allow ABCI application to handle fees tendermint/tendermint#1065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tags in DeliverTx response #130

Tags in DeliverTx response #130

melekes commented Nov 9, 2017

ebuchman left a comment

melekes commented Nov 14, 2017 •

edited

Loading

ebuchman commented Nov 14, 2017

melekes commented Nov 14, 2017

melekes commented Nov 15, 2017 •

edited

Loading

ethanfrey commented Nov 16, 2017 •

edited

Loading

ethanfrey left a comment

ethanfrey Nov 16, 2017

ethanfrey Nov 16, 2017

melekes commented Nov 16, 2017

ethanfrey commented Nov 17, 2017

ethanfrey commented Nov 17, 2017 •

edited

Loading

melekes commented Nov 17, 2017

melekes commented Nov 23, 2017

melekes Nov 23, 2017

ebuchman Nov 24, 2017

ebuchman Nov 27, 2017

melekes Nov 27, 2017

ebuchman Nov 27, 2017

melekes Nov 27, 2017

melekes Nov 27, 2017

ebuchman Nov 27, 2017

melekes Nov 27, 2017

ebuchman Nov 27, 2017

melekes Nov 27, 2017 •

edited

Loading

ebuchman Nov 27, 2017

melekes Nov 27, 2017

melekes commented Nov 27, 2017

ebuchman commented Nov 27, 2017

		@@ -6,114 +6,47 @@ import (
		"github.com/tendermint/go-wire/data"
		)

Tags in DeliverTx response #130

Tags in DeliverTx response #130

Conversation

melekes commented Nov 9, 2017

ebuchman left a comment

Choose a reason for hiding this comment

melekes commented Nov 14, 2017 • edited Loading

ebuchman commented Nov 14, 2017

melekes commented Nov 14, 2017

melekes commented Nov 15, 2017 • edited Loading

ethanfrey commented Nov 16, 2017 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes commented Nov 16, 2017

ethanfrey commented Nov 17, 2017

ethanfrey commented Nov 17, 2017 • edited Loading

melekes commented Nov 17, 2017

melekes commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes commented Nov 27, 2017

ebuchman commented Nov 27, 2017

melekes commented Nov 14, 2017 •

edited

Loading

melekes commented Nov 15, 2017 •

edited

Loading

ethanfrey commented Nov 16, 2017 •

edited

Loading

ethanfrey commented Nov 17, 2017 •

edited

Loading

melekes Nov 27, 2017 •

edited

Loading