Add Name to Events, Name to Subscription #71

vdamle · 2020-06-17T10:41:02Z

Adds name to EventStream
Adds summary to Subscription - there's already a system generated name(set using address and event signature). If no name is provided, name is set to summary in the response to the user. If a name is provided for the subscription, that name is returned in the response to the user.
Change transactionIndex in logEntry to be a hex uint instead of json Number to match https://github.com/ethereum/go-ethereum/blob/4bcc0a37ab70cb79b16893556cffdaad6974e7d8/core/types/log.go#L47. This was exposed by a behavior change in go1.14 (Resulted in a corrupted slice) when parsing invalid Number in json (see commit for more details)
Allow Solidity versions > 0.6.0 & < 0.6.9

Partial fix for: #68

* allows for solc0.6.8 * go1.14 has changes to json `Number` treatment. This resulted in some of our negative tests returning a different error message due to alphabet input for "bad number"

For correctness and also because go1.14 is messy when parsing incorrect numbers. For us, when parsing our test events from our file that has `0x0` into a "Number", the resulting slice does not have the right data. ref: golang/go#14702 https://github.com/ethereum/go-ethereum/blob/4bcc0a37ab70cb79b16893556cffdaad6974e7d8/core/types/log.go#L47

codecov · 2020-06-17T10:43:42Z

Codecov Report

Merging #71 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #71   +/-   ##
=======================================
  Coverage   97.96%   97.96%           
=======================================
  Files          50       50           
  Lines        5460     5464    +4     
=======================================
+ Hits         5349     5353    +4     
  Misses         79       79           
  Partials       32       32

Impacted Files	Coverage Δ
internal/kldevents/logprocessor.go	`100.00% <ø> (ø)`
internal/kldcontracts/rest2eth.go	`96.51% <100.00%> (+0.01%)`	⬆️
internal/kldevents/eventstream.go	`100.00% <100.00%> (ø)`
internal/kldevents/submanager.go	`100.00% <100.00%> (ø)`
internal/kldevents/subscription.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69de215...cf977ff. Read the comment docs.

peterbroadhurst

on request for a change around the customName field

Also, noticed in the PR description:

Allow Solidity versions <= 0.6.8

Think from the code, what is meant is:

Allow Solidity versions > 0.6.8

Have I understood correctly?

peterbroadhurst · 2020-06-17T18:23:05Z

internal/kldevents/subscription.go

+	ID         string                     `json:"id,omitempty"`
+	Path       string                     `json:"path"`
+	Name       string                     `json:"name"`
+	CustomName string                     `json:"customName,omitempty"`


I think it's going to be confusing to have customName here.
I'm guessing the problem was we auto-generate the name field previously from combination of the event signature and the scope - which is useful information regardless of whether a user has a custom name.

With that in mind, please can we make the new field summary, and have that always be auto-generated (cannot be user-submitted - works just like name does today). Then name defaults to the value of summary if it's empty/null, but if supplied the user can put anything they like?

Added a new field summary (not returned in API response). name is now set to whatever is provided by the user or is set to summary if it is empty.

internal/kldtx/txnprocessor_test.go

* Sets name to whatever is provided by end user, if present in the body * Sets name to `summary`, if one is not provided by end user. summary is auto generated and not returned in the API response * Removed a comment from test file based on review comment

vdamle · 2020-06-17T20:39:40Z

internal/kldtx/txnprocessor_test.go

@@ -608,7 +608,7 @@ func TestOnSendTransactionMessageBadNonce(t *testing.T) {
 	testTxnContext.jsonMsg = "{" +
 		"  \"headers\":{\"type\": \"SendTransaction\"}," +
 		"  \"from\":\"0x83dBC8e329b38cBA0Fc4ed99b1Ce9c2a390ABdC1\"," +
-		"  \"nonce\":\"abc\"" +
+		"  \"nonce\":\"123.4\"" +


Guessing this is a go version behavior change: using abc for nonce throws an error in parsing the json in the test file: "json: invalid number literal, trying to unmarshal ""abc"" into Number" instead of throwing an error in txn processing logic

vdamle · 2020-06-17T20:39:47Z

internal/kldtx/txnprocessor_test.go

@@ -630,7 +630,7 @@ func TestOnSendTransactionMessageBadMsg(t *testing.T) {
 		"  \"headers\":{\"type\": \"SendTransaction\"}," +
 		"  \"from\":\"0x83dBC8e329b38cBA0Fc4ed99b1Ce9c2a390ABdC1\"," +
 		"  \"nonce\":\"123\"," +
-		"  \"value\":\"abc\"," +
+		"  \"value\":\"123.456\"," +


Guessing this is a go version behavior change: using abc for nonce throws an error in parsing the json in the test file: "json: invalid number literal, trying to unmarshal ""abc"" into Number" instead of throwing an error in txn processing logic

vdamle

@peterbroadhurst Thanks for the review. Updated the PR with requested changes re:summary.

To your other question, the contract files do not require > 0.6.0 (I was using 0.6.8) to test locally and hence only changed the upper limit of the version.

peterbroadhurst

👍 📛 📨

The name was getting dropped in the chain of calls. Set the name that is passed down from the caller before creating the subscription info. Ref: #68 Fixes a bug introduced in #71

* Add name to Events * Allow sol 0.6.8, go1.14 tweaks for json number parsing * allows for solc0.6.8 * go1.14 has changes to json `Number` treatment. This resulted in some of our negative tests returning a different error message due to alphabet input for "bad number" * Change logEntry tx index type to hex uint For correctness and also because go1.14 is messy when parsing incorrect numbers. For us, when parsing our test events from our file that has `0x0` into a "Number", the resulting slice does not have the right data. ref: golang/go#14702 https://github.com/ethereum/go-ethereum/blob/4bcc0a37ab70cb79b16893556cffdaad6974e7d8/core/types/log.go#L47 * Add CustomName to Subscriptions * Fix json for event name * Add tests with event stream name in smartcontract gw * Sub auto-generated summary, accept name from end user * Sets name to whatever is provided by end user, if present in the body * Sets name to `summary`, if one is not provided by end user. summary is auto generated and not returned in the API response * Removed a comment from test file based on review comment Signed-off-by: Vinod Damle <vinod.damle@kaleido.io>

The name was getting dropped in the chain of calls. Set the name that is passed down from the caller before creating the subscription info. Ref: #68 Fixes a bug introduced in #71 Signed-off-by: Vinod Damle <vinod.damle@kaleido.io>

Vinod Damle added 6 commits June 12, 2020 08:45

Add name to Events

8fbe291

Allow sol 0.6.8, go1.14 tweaks for json number parsing

c319cbe

* allows for solc0.6.8 * go1.14 has changes to json `Number` treatment. This resulted in some of our negative tests returning a different error message due to alphabet input for "bad number"

Add CustomName to Subscriptions

c242e26

Fix json for event name

afeeae2

Add tests with event stream name in smartcontract gw

b5d272d

vdamle requested a review from peterbroadhurst June 17, 2020 10:41

peterbroadhurst requested changes Jun 17, 2020

View reviewed changes

vdamle commented Jun 17, 2020

View reviewed changes

peterbroadhurst approved these changes Jun 17, 2020

View reviewed changes

peterbroadhurst merged commit 1b66f30 into master Jun 17, 2020

peterbroadhurst deleted the stream-sub-name branch June 17, 2020 20:58

vdamle changed the title ~~Add Name to Events, CustomName to Subscription~~ Add Name to Events, Name to Subscription Jun 17, 2020

vdamle mentioned this pull request Jun 18, 2020

Set subscription name before creating subscription info #72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Name to Events, Name to Subscription #71

Add Name to Events, Name to Subscription #71

vdamle commented Jun 17, 2020 •

edited

Loading

codecov bot commented Jun 17, 2020 •

edited

Loading

peterbroadhurst left a comment

peterbroadhurst Jun 17, 2020

vdamle Jun 17, 2020

vdamle Jun 17, 2020

vdamle Jun 17, 2020

vdamle left a comment

peterbroadhurst left a comment

Add Name to Events, Name to Subscription #71

Add Name to Events, Name to Subscription #71

Conversation

vdamle commented Jun 17, 2020 • edited Loading

codecov bot commented Jun 17, 2020 • edited Loading

Codecov Report

peterbroadhurst left a comment

Choose a reason for hiding this comment

peterbroadhurst Jun 17, 2020

Choose a reason for hiding this comment

vdamle Jun 17, 2020

Choose a reason for hiding this comment

vdamle Jun 17, 2020

Choose a reason for hiding this comment

vdamle Jun 17, 2020

Choose a reason for hiding this comment

vdamle left a comment

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

vdamle commented Jun 17, 2020 •

edited

Loading

codecov bot commented Jun 17, 2020 •

edited

Loading