Notification filters 2.x #951
Conversation
It's not practical adding server-side tests for 2.0 (as it requires generating more blocks), so we'll leave it for 3.0.
Differing a bit from #895 draft specification, we won't add `verifier` (or signer) for Neo 2, it's not worth doing so at the moment.
Codecov Report
@@ Coverage Diff @@
## master-2.x #951 +/- ##
==============================================
- Coverage 68.71% 68.22% -0.50%
==============================================
Files 143 144 +1
Lines 13912 14172 +260
==============================================
+ Hits 9560 9669 +109
- Misses 3913 4050 +137
- Partials 439 453 +14
Continue to review full report at Codecov.
|
| drainloop: | ||
| for { | ||
| select { | ||
| case _, ok := <-subChan: |
There was a problem hiding this comment.
Why exactly do u need this ok? (instead of blocking recv)
There was a problem hiding this comment.
To be fair, it's not strictly needed as no one closes subChan, but it's a safety guard in case it changes in the future for some reason.
| default: | ||
| sub.overflown.Store(true) | ||
| // MissedEvent is to be delivered eventually. | ||
| go func(sub *subscriber) { |
There was a problem hiding this comment.
There is at most one such goroutine for each (sub, eventID) pair in each moment of time, am I right?
I care about executing .Store(false) 2 lines below while there are 2 goroutines executing concurrently.
There was a problem hiding this comment.
Actually at most one per sub as we skip overflown subscribers completely.
| if (*val).State == "HALT" || (*val).State == "FAULT" { | ||
| p.Value = *val | ||
| } else { | ||
| continue |
There was a problem hiding this comment.
Maybe there is a better fit for this JSON, not ExecutionFilter. At the moment, of course that could only be []Param which is unlikely to be a match if we're this deep with ExecutionFilter, but it also might change in the future.
pkg/rpc/server/subscription.go
Outdated
| case response.TransactionEventID: | ||
| filt := f.filter.(request.TxFilter) | ||
| tx := r.Payload[0].(*transaction.Transaction) | ||
| if tx.Type == filt.Type { |
There was a problem hiding this comment.
We can replace it with return tx.Type == filt.Type here and below, which will look more compact.
| "tx matching": { | ||
| params: `["transaction_added", {"type":"InvocationTransaction"}]`, | ||
| check: func(t *testing.T, resp *response.Notification) { | ||
| rmap := resp.Payload[0].(map[string]interface{}) |
There was a problem hiding this comment.
Why does response.Notification.Payload[0] casts here to a map and in non-test code to a specific type?
Am I missing something?
There was a problem hiding this comment.
This is a server test and it doesn't try to interpret these JSONs as real structures (it would complicate this test probably), instead it uses the same type server uses for marshaling and that has plain interface{} that is unmarshalled in standard encoding/json fashion.
pkg/core/block/block.go
Outdated
| // As Base and txes are at the same level in json, | ||
| // do unmarshalling separately for both structs. | ||
| txes := new(auxTxes) | ||
| base := new(Base) |
There was a problem hiding this comment.
Maybe allocate it after this if?
And check state string correctness on unmarshaling.
It actually was missing and it might affect Transaction conversion to/from JSON.
Our block.Block was JSONized in a bit different fashion than result.Block in its Nonce and NextConsensus fields. It's not good for notifications because third-party clients would probably expect to see the same format. Also, using completely different Block representation in result is probably making our client a bit weaker as this representation is harder to use with other neo-go components. So use the same approach we took for Transactions and wrap block.Base which is to be serialized in proper way.
The same data is copied at least three times here.
roman-khimov
force-pushed
the
notification-filters-2.x
branch
from
b5b7c47
to
8cd7bc7
Compare
Problem
#895.
Solution
This one closes #895.