Track blockchain callback handlers per namespace #883
Conversation
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
awrichar
requested review from
peterbroadhurst,
nguyer and
shorsher
as code owners
Codecov Report
@@ Coverage Diff @@
## main #883 +/- ##
=======================================
Coverage 99.96% 99.96%
=======================================
Files 299 299
Lines 19500 19568 +68
=======================================
+ Hits 19494 19562 +68
Misses 5 5
Partials 1 1
Continue to review full report at Codecov.
|
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
| if namespace == "" { | ||
| // V1 networks don't populate namespace, so deliver the event to every handler | ||
| for _, handler := range cb.handlers { | ||
| if err := handler.BlockchainNetworkAction(action, location, event, signingKey); err != nil { |
There was a problem hiding this comment.
I feel like there should be a check for delivery of this to a V2 handler, and immediate rejection (worried about V1 pre-migrated contracts leaking actions to an unrelated V2 namespace that's new/migrated and happens to share a remote-name like default). Will look out for this later in the review
There was a problem hiding this comment.
Ok, found it, in the caller of this function:
firefly/internal/blockchain/ethereum/ethereum.go
Lines 404 to 409 in 5d31178
There was a problem hiding this comment.
Yes, but you make a good point that if we can't infer the namespace from the subscription name, then V2 handlers should reject it outright. I think there should be a sanity check for that when we initialize the subscription info, to ensure that whenever we get to the point you noted above, subInfo.namespace is guaranteed to be valid for V2+ contracts.
There was a problem hiding this comment.
OK I pushed another commit to include this sanity check during init.
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (cb *callbacks) BlockchainEvent(event *blockchain.EventWithSubscription) error { | ||
| for _, cb := range cb.handlers { | ||
| // Send the event to all handlers and let them match it to a contract listener | ||
| // TODO: can we push more listener/namespace knowledge down to this layer? |
There was a problem hiding this comment.
Seems like the same namespace-encoded-in-sub-name trick should work here, right? (with a migration consideration I guess)
There was a problem hiding this comment.
(not suggesting the TODO needs to be addressed in this PR)
There was a problem hiding this comment.
Yea, thanks for the prod - I need to add that follow-up to the board. Currently our listeners for custom events have no significance to their subscription names (it's just ff-sub-<listener UUID>, but it really doesn't matter at all). We should introduce a format that can be parsed (with a fallback to "send it to everyone" for pre-existing listeners).
| if err := cb.BlockchainNetworkAction(action, event, signingKey); err != nil { | ||
| return err | ||
| func (cb *callbacks) BlockchainNetworkAction(ctx context.Context, namespace, action string, location *fftypes.JSONAny, event *blockchain.Event, signingKey *core.VerifierRef) error { | ||
| if namespace == "" { |
There was a problem hiding this comment.
Can't help but (re) note the code duplication here - fine by me if your assessment is it's still the right thing.
There was a problem hiding this comment.
Yea, we were able to commonize a chunk of the init and ingress logic for the FireFly multiparty contract when Multiparty Manager came into being - but you're right that the other end (handling the events that pop out of the multiparty contract) still has a lot of duplication. I'll note it for another pass on whether more of it can be commonized into Event Manger or Multiparty Manager (or some new place).
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Part of FIR-12
For blockchain batch pin events, direct them to an appropriate namespace:
For blockchain network actions, direct them to an appropriate namespace:
Caveat for pre-existing subscriptions: Old subscription names will not have a namespace included, so multiple namespaces will actually be sharing a single subscription. This is OK for V1 networks, as batch pin events always contain a namespace and network actions are sent to all handlers. A V2 network cannot function in this situation, so V2 contracts will be fundamentally incompatible with releases of FireFly prior to 1.1.
Tokens are also subject to the same problem where pre-existing subscriptions will not include namespace information, so they always send events to all handlers if namespace is unset.
Unanswered question: how does ff_system "go away" cleanly when migrating to a V2 contract? There is no entry for ff_system in the namespace config, so migration is weird and probably tied to the default namespace instead. UPDATE - see #884