Get block notifications API #3781
Conversation
| return nil, respErr | ||
| } | ||
|
|
||
| block, err := s.chain.GetBlock(hash) |
There was a problem hiding this comment.
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.
| for _, tx := range block.Transactions { | ||
| aers, err := s.chain.GetAppExecResults(tx.Hash(), trigger.Application) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
Return internal error? This means some DB inconsistency and the result can be incorrect.
| } | ||
| } | ||
|
|
||
| // Also check for notifications from the block itself (PostPersist) |
There was a problem hiding this comment.
You need to keep the sequence correct and check for trigger.OnPersist before digging into transaction list.
There was a problem hiding this comment.
We may additionally extend neorpc.NotificaitonFilter with trigger parameter, it may be useful in some cases, what do you think?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Also check for notifications from the block itself (PostPersist) | ||
| aers, err := s.chain.GetAppExecResults(block.Hash(), trigger.Application) |
| aers, err := s.chain.GetAppExecResults(block.Hash(), trigger.Application) | ||
| if err == nil && len(aers) > 0 { | ||
| aer := aers[0] | ||
| if aer.VMState == vmstate.Halt { |
There was a problem hiding this comment.
Maybe this code can be generalized in some way (helper function?), it's the same for blocks/transactions.
| } | ||
|
|
||
| // testComparator is a helper type for notification filtering | ||
| type testComparator struct { |
There was a problem hiding this comment.
You can move these out into a separate file (along with the logic for appending to notifications.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Please check for expected contents as well.
|
RPC client needs to be updated to support this as well. |
| for _, tx := range block.Transactions { | ||
| aers, err := s.chain.GetAppExecResults(tx.Hash(), trigger.Application) | ||
| if err != nil { | ||
| continue |
| } | ||
| } | ||
|
|
||
| // Also check for notifications from the block itself (PostPersist) |
There was a problem hiding this comment.
We may additionally extend neorpc.NotificaitonFilter with trigger parameter, it may be useful in some cases, what do you think?
| } | ||
|
|
||
| // testComparator is a helper type for notification filtering | ||
| type testComparator struct { |
There was a problem hiding this comment.
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.
| }, | ||
| { | ||
| name: "positive with filter", | ||
| params: `["` + genesisBlockHash + `", {"contract":"` + testContractHashLE + `", "name":"Transfer"}]`, |
There was a problem hiding this comment.
Genesis block doesn't contain notifications from testContract. Let's use native Gas or Neo hash as a filter.
| @@ -219,6 +220,7 @@ var rpcHandlers = map[string]func(*Server, params.Params) (any, *neorpc.Error){ | |||
| "getblockhash": (*Server).getBlockHash, | |||
| "getblockheader": (*Server).getBlockHeader, | |||
| "getblockheadercount": (*Server).getBlockHeaderCount, | |||
| "getblocknotifications": (*Server).getBlockNotifications, | |||
There was a problem hiding this comment.
This extension should be documented at docs/rpc.md, see ### Extensions section.
| @@ -219,6 +220,7 @@ var rpcHandlers = map[string]func(*Server, params.Params) (any, *neorpc.Error){ | |||
| "getblockhash": (*Server).getBlockHash, | |||
| "getblockheader": (*Server).getBlockHeader, | |||
| "getblockheadercount": (*Server).getBlockHeaderCount, | |||
| "getblocknotifications": (*Server).getBlockNotifications, | |||
There was a problem hiding this comment.
This extension should be supported at the RPC client side (pkg/rpcclient/rpc.go).
|
Please, ensure testing jobs are passing. Currently, unit-tests, project linter and DCO checks are failing. |
Closes #3779.
It works but I don't know if this is the best way to do it.