go/consensus/api: Add `GetTransactionsWithResults` method #3073

ptrus · 2020-07-01T07:18:20Z

codecov · 2020-07-01T07:46:34Z

Codecov Report

Merging #3073 into master will increase coverage by 0.03%.
The diff coverage is 66.06%.

@@            Coverage Diff             @@
##           master    #3073      +/-   ##
==========================================
+ Coverage   68.17%   68.21%   +0.03%     
==========================================
  Files         371      372       +1     
  Lines       36546    36578      +32     
==========================================
+ Hits        24917    24950      +33     
+ Misses       8416     8400      -16     
- Partials     3213     3228      +15

Impacted Files	Coverage Δ
go/consensus/api/api.go	`50.00% <ø> (ø)`
go/consensus/api/transaction/results/results.go	`0.00% <0.00%> (ø)`
go/consensus/tendermint/api/api.go	`74.54% <ø> (-4.87%)`	⬇️
go/registry/api/api.go	`41.65% <ø> (ø)`
go/roothash/api/api.go	`75.00% <ø> (ø)`
go/consensus/tendermint/roothash/roothash.go	`61.69% <51.35%> (-6.81%)`	⬇️
go/consensus/tendermint/registry/registry.go	`75.57% <61.79%> (+6.31%)`	⬆️
go/consensus/tendermint/tendermint.go	`65.57% <67.34%> (-0.14%)`	⬇️
go/consensus/tendermint/staking/staking.go	`68.63% <67.69%> (+4.09%)`	⬆️
go/consensus/api/grpc.go	`68.72% <76.47%> (+0.51%)`	⬆️
... and 42 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 91b6ddb...37576a9. Read the comment docs.

go/consensus/api/api.go

kostko

Nice, this recent change looks good! Feel free to do the same for other services (registry, roothash). We could probably also get rid of ConvertBlockEvents and the associated types then?

go/consensus/api/transaction/results/results.go

go/consensus/tendermint/staking/staking.go

go/roothash/api/api.go

ptrus · 2020-07-06T11:50:14Z

go/oasis-node/cmd/debug/txsource/workload/queries.go

@@ -69,6 +72,87 @@ type queries struct {
 	runtimeGenesisRound uint64
 }

+func (q *queries) sanityCheckTransactionEvents(ctx context.Context, height int64, txEvents []*results.Event) error {
+	// Ensure transaction events match querying backend GetEvents methods.
+	// TODO: Registry events don't include any transaction info, so we cannot


Should we add this (e.g. adding some tx info - maybe the transaction hash as we do for staking, to registry events as well)?

Yeah we should.

ptrus · 2020-07-06T11:51:42Z

@kostko this is ready for more reviews, i posted 2 questions as comments (will also split into multiple commits & fragments after this round of reviews)

go/registry/api/api.go

go/staking/api/api.go

kostko · 2020-07-06T12:27:25Z

go/oasis-node/cmd/debug/txsource/workload/queries.go

@@ -69,6 +72,87 @@ type queries struct {
 	runtimeGenesisRound uint64
 }

+func (q *queries) sanityCheckTransactionEvents(ctx context.Context, height int64, txEvents []*results.Event) error {
+	// Ensure transaction events match querying backend GetEvents methods.
+	// TODO: Registry events don't include any transaction info, so we cannot


Yeah we should.

go/roothash/api/api.go

go/consensus/tendermint/roothash/roothash.go

ptrus · 2020-07-07T12:22:11Z

go/consensus/tendermint/roothash/roothash.go

+				events = append(events, ev)
+			case bytes.Equal(key, app.KeyRuntimeID):
+				// Runtime ID attribute.
+				// TODO: Every event should contain this attribute, but not used atm


See this comments please @kostko (should we remove the RuntimeID field in each of the events, now that we have this attribute? or should we keep it unused for now, maybe just add a check for it to exist for every event?)

So the RuntimeID attribute would mostly be used for filtering events in queries (in this case the runtime ID that it contains as value should be text-serialized) not for populating it from anywhere else. So it is fine if it is unused during processing (could add a sanity check to make sure it is there though).

kostko

Nice overall, added some minor comments.

go/registry/api/api.go

kostko · 2020-07-07T12:38:48Z

go/registry/api/api.go

+}
+
+// InternalEvent is a registry app internal event that is not exposed.
+type InternalEvent struct {


Why even expose this in the general API? If it is internal then it is backend-specific so it should just be defined inside the backend implementation and not exposed.

yeah agreed & fixed

go/roothash/api/api.go

go/consensus/tendermint/roothash/roothash.go

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch 2 times, most recently from 2363932 to 654169b Compare July 1, 2020 07:31

kostko reviewed Jul 1, 2020

View reviewed changes

go/consensus/api/api.go Outdated Show resolved Hide resolved

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch from 654169b to 4bb3b28 Compare July 1, 2020 14:09

kostko reviewed Jul 1, 2020

View reviewed changes

ptrus mentioned this pull request Jul 2, 2020

Consider removing specific staking event notifiers #3080

Closed

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch 23 times, most recently from 4ef3cb9 to 0e28065 Compare July 6, 2020 10:59

ptrus commented Jul 6, 2020

View reviewed changes

go/roothash/api/api.go Show resolved Hide resolved

ptrus commented Jul 6, 2020

View reviewed changes

kostko reviewed Jul 6, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch 5 times, most recently from fbd7613 to 5772c59 Compare July 7, 2020 12:16

ptrus marked this pull request as ready for review July 7, 2020 12:18

ptrus requested review from peterjgilbert, pro-wh and Yawning as code owners July 7, 2020 12:18

ptrus commented Jul 7, 2020

View reviewed changes

kostko reviewed Jul 7, 2020

View reviewed changes

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch 2 times, most recently from 2be39b1 to e4c3629 Compare July 7, 2020 14:22

kostko approved these changes Jul 7, 2020

View reviewed changes

ptrus added 4 commits July 7, 2020 19:47

go/tendermint/roothash: include RuntimeID attribute in events

5176041

go/roothash/api: Include RuntimeID field in roothash events

50ff772

go/registry/api: Include transaction hash and height in registry events

681b14d

go/consensus/api: Add GetTransactionsWithResults method

37576a9

ptrus force-pushed the ptrus/feature/transaction-results-rpc branch from e4c3629 to 37576a9 Compare July 7, 2020 17:48

ptrus merged commit 451b663 into master Jul 7, 2020

ptrus deleted the ptrus/feature/transaction-results-rpc branch July 7, 2020 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/consensus/api: Add `GetTransactionsWithResults` method #3073

go/consensus/api: Add `GetTransactionsWithResults` method #3073

ptrus commented Jul 1, 2020 •

edited

Loading

codecov bot commented Jul 1, 2020 •

edited

Loading

kostko left a comment

ptrus Jul 6, 2020

kostko Jul 6, 2020

ptrus commented Jul 6, 2020

kostko Jul 6, 2020

ptrus Jul 7, 2020

kostko Jul 7, 2020 •

edited

Loading

kostko left a comment

kostko Jul 7, 2020

ptrus Jul 7, 2020

go/consensus/api: Add GetTransactionsWithResults method #3073

go/consensus/api: Add GetTransactionsWithResults method #3073

Conversation

ptrus commented Jul 1, 2020 • edited Loading

codecov bot commented Jul 1, 2020 • edited Loading

Codecov Report

kostko left a comment

Choose a reason for hiding this comment

ptrus Jul 6, 2020

Choose a reason for hiding this comment

kostko Jul 6, 2020

Choose a reason for hiding this comment

ptrus commented Jul 6, 2020

kostko Jul 6, 2020

Choose a reason for hiding this comment

ptrus Jul 7, 2020

Choose a reason for hiding this comment

kostko Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

kostko left a comment

Choose a reason for hiding this comment

kostko Jul 7, 2020

Choose a reason for hiding this comment

ptrus Jul 7, 2020

Choose a reason for hiding this comment

go/consensus/api: Add `GetTransactionsWithResults` method #3073

go/consensus/api: Add `GetTransactionsWithResults` method #3073

ptrus commented Jul 1, 2020 •

edited

Loading

codecov bot commented Jul 1, 2020 •

edited

Loading

kostko Jul 7, 2020 •

edited

Loading