tx resource contains operations #863

kfangw · 2018-12-03T07:45:47Z

Github Issue

#862

Background

see #862

Solution

Transaction Resource contains operations

Possible Drawbacks

codecov-io · 2018-12-03T07:51:43Z

Codecov Report

Merging #863 into master will increase coverage by 1.95%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##           master     #863      +/-   ##
==========================================
+ Coverage   58.21%   60.16%   +1.95%     
==========================================
  Files         154      154              
  Lines       10996    11003       +7     
==========================================
+ Hits         6401     6620     +219     
+ Misses       3847     3607     -240     
- Partials      748      776      +28

Flag	Coverage Δ
#integration_tests_long_term	`44.05% <0%> (?)`
#integration_tests_node	`40.02% <0%> (-0.03%)`	⬇️
#unittests	`48.44% <52.94%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
lib/node/runner/api/api.go	`85% <ø> (+24.28%)`	⬆️
lib/node/runner/api/stream.go	`42.62% <0%> (-6.91%)`	⬇️
lib/node/runner/api/resource/transaction.go	`59.45% <100%> (+2.31%)`	⬆️
lib/node/runner/api/resource/operation.go	`83.33% <100%> (+0.57%)`	⬆️
lib/node/runner/api/transaction.go	`58.71% <69.56%> (-7.33%)`	⬇️
cmd/sebak/cmd/run.go	`57.44% <0%> (+0.35%)`	⬆️
lib/node/runner/isaac_state_manager.go	`94.31% <0%> (+1.42%)`	⬆️
lib/node/runner/api_block.go	`67.5% <0%> (+1.66%)`	⬆️
lib/block/transaction.go	`70.12% <0%> (+1.82%)`	⬆️
... and 12 more

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 359ae36...824fd71. Read the comment docs.

kfangw

note

kfangw · 2018-12-03T09:02:46Z

lib/node/runner/api/api.go

@@ -106,27 +103,3 @@ func TriggerEvent(st *storage.LevelDBBackend, transactions []*transaction.Transa
 	}

 }
-
-func renderEventStream(args ...interface{}) ([]byte, error) {


the renderEventStream function is used only by the subscribe API
and it is impossible to get data from storage with this design.
Hence, this function is moved into subscribe API handler and let the function use the closure.

kfangw · 2018-12-03T09:04:26Z

lib/node/runner/api/resource/transaction.go

@@ -27,6 +31,7 @@ func (t Transaction) GetMap() hal.Entry {
 		"sequence_id":     t.bt.SequenceID,
 		"created":         t.bt.Created,
 		"operation_count": len(t.bt.Operations),
+		"operations":      t.tx.B.Operations,


the shape of operation in the transaction resource may differ from operation resource.
But I think that the operation resource will be deprecated and relevant APIs no longer under maintenance.

kfangw · 2018-12-03T09:21:38Z

review please @spikeekips @anarcher @Geod24

lib/node/runner/api/resource/transaction.go

lib/node/runner/api/stream.go

Geod24 · 2018-12-03T10:20:23Z

lib/node/runner/api/stream.go

+		}
+		i := args[1]
+
+		if i == nil {


This check should be folded into line 47 so the error is the same

line 47 checks the length of arguments
and this line is for checking the first argument value is nil

Yes, but making a difference here does not matter and only looks confusing. Having both checks done together is more consistent.

lib/node/runner/api/stream.go

Geod24 · 2018-12-03T10:23:41Z

lib/node/runner/api/transaction.go

+		t, hasNext, c := iterFunc()
+		if !hasNext {
+			break
+		}


This should be part of the for condition, instead of infinite loop + break

It is a matter of style.
And I was trying to make it for init; cond; after {} style,
the init value is deallocated when the for loop is finished.
I mean, the t is deallocated.

I mean, the t is deallocated.

It goes out of scope. But so does it, with the current code. So I don't see the point.

Geod24 · 2018-12-03T10:26:24Z

lib/node/runner/api/transaction.go

-		}
-		payload = resource.NewTransaction(&bt)
-		return payload, nil
+	found, err := block.ExistsBlockTransaction(api.storage, key)


Instead of exists + get, why don't you get it directly ?

To distinguish error in the database and no error but not exist.

The underlying method already does that

kfangw · 2018-12-06T06:48:03Z

Ping @anarcher @spikeekips @Geod24

kfangw · 2018-12-07T06:56:41Z

@cmcm2222 When this merged, please update the API document according to this change.

kfangw · 2018-12-11T08:14:57Z

ping @anarcher @Geod24

anarcher

LGTM!

kfangw · 2018-12-12T05:29:18Z

@cmcm2222 Could you test this PR and update the Document?
I want to merge this when you are ready for the document.

kfangw · 2018-12-18T04:13:21Z

@cmcm2222 Could you test this PR and update the Document?
I want to merge this when you are ready for the document.

@cmcm2222 ping

johndoe0x · 2018-12-18T08:17:07Z

@kfangw OK. I will check it

Do not use Operation Hash for validation Identifier the operation is changed to Tx + Index

update API doc based on bosnet/sebak#863

kfangw changed the title ~~[WIP] tx resource contains operations~~ tx resource contains operations Dec 3, 2018

kfangw force-pushed the 862_tx_operations branch 2 times, most recently from bb4157f to 0cf875b Compare December 3, 2018 08:49

kfangw self-assigned this Dec 3, 2018

kfangw added the API label Dec 3, 2018

kfangw mentioned this pull request Dec 3, 2018

Transactions sometimes contain duplicate operations #854

Open

kfangw force-pushed the 862_tx_operations branch from 0cf875b to 9797c81 Compare December 3, 2018 09:00

kfangw commented Dec 3, 2018

View reviewed changes

kfangw requested review from anarcher, Geod24 and spikeekips December 3, 2018 09:21

Geod24 reviewed Dec 3, 2018

View reviewed changes

kfangw force-pushed the 862_tx_operations branch from b141b77 to c10c002 Compare December 5, 2018 05:37

spikeekips approved these changes Dec 7, 2018

View reviewed changes

kfangw added the Doc All that Documentation label Dec 7, 2018

kfangw requested a review from johndoe0x December 7, 2018 06:56

anarcher approved these changes Dec 11, 2018

View reviewed changes

kfangw mentioned this pull request Dec 12, 2018

[Proposal] API endpoint to get operation details by hash #877

Closed

kfangw added 3 commits January 10, 2019 15:16

Do not use Operation Hash for validation (bosnet#850)

0ee9bea

Do not use Operation Hash for validation Identifier the operation is changed to Tx + Index

fmt

3f7e6b1

index

f89b71e

kfangw force-pushed the 862_tx_operations branch from 5daf848 to f89b71e Compare January 10, 2019 06:16

Merge branch 'master' into 862_tx_operations

824fd71

kfangw merged commit 6e4cc0f into bosnet:master Jan 14, 2019

kfangw deleted the 862_tx_operations branch January 14, 2019 02:05

kfangw mentioned this pull request Jan 14, 2019

[API] Make Transaction resource contains Operations #862

Closed

johndoe0x pushed a commit to bosnet/docs that referenced this pull request Jan 31, 2019

Merge pull request #23 from cmcm2222/master

c8117d8

update API doc based on bosnet/sebak#863

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tx resource contains operations #863

tx resource contains operations #863

kfangw commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 3, 2018 •

edited

Loading

kfangw left a comment

kfangw Dec 3, 2018

kfangw Dec 3, 2018

kfangw commented Dec 3, 2018

Geod24 Dec 3, 2018

kfangw Dec 4, 2018

Geod24 Dec 13, 2018

Geod24 Dec 3, 2018

kfangw Dec 4, 2018

Geod24 Dec 13, 2018

Geod24 Dec 3, 2018

kfangw Dec 4, 2018

Geod24 Dec 13, 2018

kfangw commented Dec 6, 2018

kfangw commented Dec 7, 2018

kfangw commented Dec 11, 2018

anarcher left a comment

kfangw commented Dec 12, 2018

kfangw commented Dec 18, 2018

johndoe0x commented Dec 18, 2018

tx resource contains operations #863

tx resource contains operations #863

Conversation

kfangw commented Dec 3, 2018 • edited Loading

Github Issue

Background

Solution

Possible Drawbacks

codecov-io commented Dec 3, 2018 • edited Loading

Codecov Report

kfangw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfangw commented Dec 3, 2018

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

kfangw commented Dec 6, 2018

kfangw commented Dec 7, 2018

kfangw commented Dec 11, 2018

anarcher left a comment

Choose a reason for hiding this comment

kfangw commented Dec 12, 2018

kfangw commented Dec 18, 2018

johndoe0x commented Dec 18, 2018

kfangw commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 3, 2018 •

edited

Loading