Get block notifications API #3781

lock9 · 2025-01-06T19:09:18Z

Closes #3779.

It works but I don't know if this is the best way to do it.

roman-khimov

Nice one overall.

roman-khimov · 2025-01-07T09:05:03Z

pkg/services/rpcsrv/server.go

+		return nil, respErr
+	}
+
+	block, err := s.chain.GetBlock(hash)


I think we need some GetTrimmedBlock or alike, this one does extract all transactions and we don't need them (only hash is needed for GetAppExecResults()). But it's an optimization in its essence, current code will work fine.

roman-khimov · 2025-01-07T09:09:00Z

pkg/services/rpcsrv/server.go

+	for _, tx := range block.Transactions {
+		aers, err := s.chain.GetAppExecResults(tx.Hash(), trigger.Application)
+		if err != nil {
+			continue


Return internal error? This means some DB inconsistency and the result can be incorrect.

Vote up for error return.

roman-khimov · 2025-01-07T09:10:36Z

pkg/services/rpcsrv/server.go

+		}
+	}
+
+	// Also check for notifications from the block itself (PostPersist)


You need to keep the sequence correct and check for trigger.OnPersist before digging into transaction list.

We may additionally extend neorpc.NotificaitonFilter with trigger parameter, it may be useful in some cases, what do you think?

What are the use cases? People interested in block-level things (and not a lot happens there) probably pull block applogs directly and otherwise it's all Application.

pkg/services/rpcsrv/server.go

roman-khimov · 2025-01-07T09:12:17Z

pkg/services/rpcsrv/server.go

+}
+
+// testComparator is a helper type for notification filtering
+type testComparator struct {


You can move these out into a separate file (along with the logic for appending to notifications.

Also, I'd say that testComparator/testContainer is suitable for testing code, but not for the production code. Let's rename it to something more appropriate.

roman-khimov · 2025-01-07T09:12:47Z

pkg/services/rpcsrv/server_test.go

+			params: `["` + genesisBlockHash + `", {"contract":"` + testContractHashLE + `", "name":"Transfer"}]`,
+			result: func(e *executor) any { return []state.ContainedNotificationEvent{} },
+			check: func(t *testing.T, e *executor, acc any) {
+				res, ok := acc.([]state.ContainedNotificationEvent)


Please check for expected contents as well.

roman-khimov · 2025-01-09T08:04:16Z

RPC client needs to be updated to support this as well.

AnnaShaleva

A useful extension.

AnnaShaleva · 2025-01-09T07:53:09Z

pkg/services/rpcsrv/server.go

+	for _, tx := range block.Transactions {
+		aers, err := s.chain.GetAppExecResults(tx.Hash(), trigger.Application)
+		if err != nil {
+			continue


Vote up for error return.

AnnaShaleva · 2025-01-09T10:25:24Z

pkg/services/rpcsrv/server.go

+		}
+	}
+
+	// Also check for notifications from the block itself (PostPersist)


We may additionally extend neorpc.NotificaitonFilter with trigger parameter, it may be useful in some cases, what do you think?

AnnaShaleva · 2025-01-09T10:29:29Z

pkg/services/rpcsrv/server.go

+}
+
+// testComparator is a helper type for notification filtering
+type testComparator struct {


Also, I'd say that testComparator/testContainer is suitable for testing code, but not for the production code. Let's rename it to something more appropriate.

pkg/services/rpcsrv/server_test.go

pkg/services/rpcsrv/server.go

AnnaShaleva · 2025-01-09T10:36:07Z

Please, ensure testing jobs are passing. Currently, unit-tests, project linter and DCO checks are failing.

lock9 · 2025-01-15T18:11:43Z

Hey guys, got some busy days ahead. I'll come back to this in one or two weeks max. Sorry for the delay

lock9 · 2025-01-20T06:57:50Z

PR Updated.

Changes:

Added GetTrimmedBlock and TrimmedBlock
- Note: There's already the concept of 'trimmed block' and this name may not be ideal
Added a new struct "BlockNotifications", grouping notifications by trigger type
Updated the tests to use the neo token hash
Updated the tests to check the notification content
Changed the name of some functions/types
Moved reusable logic to the utils file
Added the new endpoint to the rpc docs file
Updated the client class to support the new endpoint

Still pending:

RPC Client tests

About trigger filters: I don't know/don't have an opinion.

lock9 · 2025-01-20T07:24:18Z

pkg/core/block/block.go

+	return nil
+}
+
+func (b *TrimmedBlock) ToStackItem() stackitem.Item {


is this necessary?

No, remove it. We're only interested in proper deserialization from bytes.

codecov · 2025-01-20T09:40:58Z

Codecov Report

Attention: Patch coverage is 61.25000% with 62 lines in your changes missing coverage. Please review.

Project coverage is 82.99%. Comparing base (16a6a59) to head (d012a6b).

Files with missing lines	Patch %	Lines
pkg/core/block/block.go	45.33%	33 Missing and 8 partials ⚠️
pkg/services/rpcsrv/server.go	67.56%	9 Missing and 3 partials ⚠️
pkg/rpcclient/rpc.go	0.00%	6 Missing ⚠️
pkg/core/dao/dao.go	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3781      +/-   ##
==========================================
- Coverage   83.14%   82.99%   -0.16%     
==========================================
  Files         335      335              
  Lines       46896    47056     +160     
==========================================
+ Hits        38994    39054      +60     
- Misses       6310     6399      +89     
- Partials     1592     1603      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnnaShaleva · 2025-01-20T10:34:36Z

docs/rpc.md

@@ -248,6 +248,10 @@ block. It can be removed in future versions, but at the moment you can use it
 to see how much GAS is burned with a particular block (because system fees are
 burned).

+#### `getblocknotifications` call
+
+This method returns notifications from a block organized by trigger type. Supports filtering by contract and event name.


Please, preserve common line width (~80 symbols).

AnnaShaleva · 2025-01-20T11:19:09Z

pkg/core/block/block.go

@@ -221,7 +235,6 @@ func (b *Block) GetExpectedBlockSizeWithoutTransactions(txCount int) int {
 	return size
 }

-// ToStackItem converts Block to stackitem.Item.


Useless change.

AnnaShaleva · 2025-01-20T11:32:44Z

pkg/core/block/block.go

+type auxTrimmedBlockOut struct {
+	TxHashes []util.Uint256 `json:"tx"`
+}
+
+// auxTrimmedBlockIn is used for JSON i/o.
+type auxTrimmedBlockIn struct {
+	TxHashes []json.RawMessage `json:"tx"`
+}


JSON marshallers are not needed for this structure. DecodeBinary is enough for our purposes.

AnnaShaleva · 2025-01-20T11:33:47Z

pkg/core/block/block.go

+	return block, br.Err
+}
+
+func (b TrimmedBlock) MarshalJSON() ([]byte, error) {


Also should be removed, why do we need this code?

At first, I considered adding a get trimmed block endpoint. I also had some issues to debug the RPC endpoints (it doesn't stop on the debugger). I had an error that I didn't know where it was coming from. Later I realized that it was due to the notification object serialization, not the block. The same for the 'toStackItem'. I didn't know if that was being used (now I know).

I also got confused because the tests 'were passing', when in practice, they weren't. I 'suspected' that this was related to some inner serilization / deserialization that I wasn't aware of. That's why I included these methods. I'll remove this and other parts that aren't being used.

AnnaShaleva · 2025-01-20T11:34:27Z

pkg/core/block/block.go

+	return nil
+}
+
+func (b *TrimmedBlock) ToStackItem() stackitem.Item {


No, remove it. We're only interested in proper deserialization from bytes.

AnnaShaleva · 2025-01-20T12:31:05Z

pkg/services/rpcsrv/util.go

@@ -18,3 +24,46 @@ func checkInt32(i int) error {
 	}
 	return nil
 }
+
+func (c notificationComparatorFilter) EventID() neorpc.EventID {


All exported methods require a comment.

AnnaShaleva · 2025-01-20T12:35:00Z

pkg/services/rpcsrv/util.go

@@ -18,3 +24,46 @@ func checkInt32(i int) error {
 	}
 	return nil
 }
+
+func (c notificationComparatorFilter) EventID() neorpc.EventID {


We're trying to avoid util.go files as much as possible. Let's create a dedicated file for these structures and name this file accordingly.

AnnaShaleva · 2025-01-20T12:38:08Z

pkg/services/rpcsrv/util.go

@@ -18,3 +24,46 @@ func checkInt32(i int) error {
 	}
 	return nil
 }
+
+func (c notificationComparatorFilter) EventID() neorpc.EventID {


s/notificationComparatorFilter/notificationEventComparator

AnnaShaleva · 2025-01-20T12:40:34Z

pkg/services/rpcsrv/util.go

@@ -18,3 +24,46 @@ func checkInt32(i int) error {
 	}
 	return nil
 }
+
+func (c notificationComparatorFilter) EventID() neorpc.EventID {
+	return c.id


c.id field is not needed, remove it. notificatoinEventComparator works only with ContainedNotificatoinEvent, hence you may return neorpc.NotificationEventID here.

AnnaShaleva · 2025-01-20T12:51:13Z

pkg/services/rpcsrv/util.go

+			Container:         container,
+			NotificationEvent: evt,
+		}
+		if filter == nil || rpcevent.Matches(&notificationComparatorFilter{


filter == nil check is excessive. rpcevent.Matche perfectly handle this case.

AnnaShaleva · 2025-01-20T12:54:16Z

About trigger filters: I don't know/don't have an opinion.

Seems that we don't need it for now, so nothing should be done wrt this question.

AnnaShaleva · 2025-01-20T12:57:17Z

Please, format commits according to https://github.com/nspcc-dev/.github/blob/master/git.md#commits. See https://github.com/nspcc-dev/neo-go/pull/3787/commits for example.

Also, please ensure your commits have valid signed-off-by signature.

lock9 requested review from AnnaShaleva and roman-khimov as code owners January 6, 2025 19:09

roman-khimov requested changes Jan 7, 2025

View reviewed changes

AnnaShaleva reviewed Jan 9, 2025

View reviewed changes

lock9 force-pushed the block-notifications branch from c1849a5 to dee62f4 Compare January 20, 2025 06:32

Get block notifications API

cb76197

lock9 force-pushed the block-notifications branch from dee62f4 to cb76197 Compare January 20, 2025 06:34

Lint

fe5e716

Removing comments in portuguese

d012a6b

lock9 commented Jan 20, 2025

View reviewed changes

AnnaShaleva reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get block notifications API #3781

Get block notifications API #3781

lock9 commented Jan 6, 2025

roman-khimov left a comment

roman-khimov Jan 7, 2025

roman-khimov Jan 7, 2025

AnnaShaleva Jan 9, 2025

roman-khimov Jan 7, 2025

AnnaShaleva Jan 9, 2025

roman-khimov Jan 9, 2025

roman-khimov Jan 7, 2025

AnnaShaleva Jan 9, 2025

roman-khimov Jan 7, 2025

roman-khimov commented Jan 9, 2025

AnnaShaleva left a comment

AnnaShaleva Jan 9, 2025

AnnaShaleva Jan 9, 2025

AnnaShaleva Jan 9, 2025

AnnaShaleva commented Jan 9, 2025

lock9 commented Jan 15, 2025

lock9 commented Jan 20, 2025

lock9 Jan 20, 2025

AnnaShaleva Jan 20, 2025

codecov bot commented Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

lock9 Jan 20, 2025 •

edited

Loading

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva Jan 20, 2025

AnnaShaleva commented Jan 20, 2025

AnnaShaleva commented Jan 20, 2025

Get block notifications API #3781

Are you sure you want to change the base?

Get block notifications API #3781

Conversation

lock9 commented Jan 6, 2025

roman-khimov left a 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

Choose a reason for hiding this comment

roman-khimov commented Jan 9, 2025

AnnaShaleva left a 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

AnnaShaleva commented Jan 9, 2025

lock9 commented Jan 15, 2025

lock9 commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2025

Codecov Report

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

lock9 Jan 20, 2025 • edited Loading

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

AnnaShaleva commented Jan 20, 2025

AnnaShaleva commented Jan 20, 2025

lock9 Jan 20, 2025 •

edited

Loading