Enable contract listeners with multiple filters #1418

nguyer · 2023-10-12T20:56:15Z

This PR adds the ability to listen to multiple types of events on the same contract listener, by adding an array of listeners, rather than a single event signature/location per listener. The old way of creating a listener is still accepted by the API, but it will always be returned in the filters array now. This is a migration concern that needs to be documented.

Open questions

One thing I'm not sure about here, is that this PR as-is removes the uniqueness constraint on listeners by topic/location/signature. It now allows multiples. I'm not sure if this is a problem or not. I can add that constraint back, but it would likely require some more sophisticated DB changes. Which brings me to the next point...

Right now all the filters for a contract listener get serialized to JSON and stored in a single column. I lated realized this means we lose the ability to query/filter (no pun intended) by signature, location, etc. which we used to do, in order to check for duplicates. I'm not sure if this is required or not, but wanted to call it out.

Example

Create contract listener request

{
    "filters": [
        {
            "interface": {
                "id": "aaa0e410-2b5b-4815-a80a-a18f2ae59f7d"
            },
            "eventPath": "BatchPin",
            "location": {
                "address": "0xb0cd60ade460e797e0c9d206290ac4ed45672c60"
            }
        }
    ],
    "name": "CustomBatchPin",
    "options": {
        "firstEvent": "oldest"
    },
    "topic": "batch-pin"
}

Create contract listener response

{
    "id": "acc0d227-1da4-4d0d-bbe0-0c60f754158f",
    "namespace": "default",
    "name": "CustomBatchPin",
    "backendId": "018b258a-0c2c-07c0-5d59-50583ae91f1e",
    "created": "2023-10-12T20:18:06.012167Z",
    "filters": [
        {
            "event": {
                "name": "BatchPin",
                "description": "",
                "params": [
                    {
                        "name": "author",
                        "schema": {
                            "type": "string",
                            "details": {
                                "type": "address",
                                "internalType": "address"
                            },
                            "description": "A hex encoded set of bytes, with an optional '0x' prefix"
                        }
                    },
                    {
                        "name": "timestamp",
                        "schema": {
                            "oneOf": [
                                {
                                    "type": "string"
                                },
                                {
                                    "type": "integer"
                                }
                            ],
                            "details": {
                                "type": "uint256",
                                "internalType": "uint256"
                            },
                            "description": "An integer. You are recommended to use a JSON string. A JSON number can be used for values up to the safe maximum."
                        }
                    },
                    {
                        "name": "action",
                        "schema": {
                            "type": "string",
                            "details": {
                                "type": "string",
                                "internalType": "string"
                            }
                        }
                    },
                    {
                        "name": "uuids",
                        "schema": {
                            "type": "string",
                            "details": {
                                "type": "bytes32",
                                "internalType": "bytes32"
                            },
                            "description": "A hex encoded set of bytes, with an optional '0x' prefix"
                        }
                    },
                    {
                        "name": "batchHash",
                        "schema": {
                            "type": "string",
                            "details": {
                                "type": "bytes32",
                                "internalType": "bytes32"
                            },
                            "description": "A hex encoded set of bytes, with an optional '0x' prefix"
                        }
                    },
                    {
                        "name": "payloadRef",
                        "schema": {
                            "type": "string",
                            "details": {
                                "type": "string",
                                "internalType": "string"
                            }
                        }
                    },
                    {
                        "name": "contexts",
                        "schema": {
                            "type": "array",
                            "details": {
                                "type": "bytes32[]",
                                "internalType": "bytes32[]"
                            },
                            "items": {
                                "type": "string",
                                "description": "A hex encoded set of bytes, with an optional '0x' prefix"
                            }
                        }
                    }
                ]
            },
            "location": {
                "address": "0xb0cd60ade460e797e0c9d206290ac4ed45672c60"
            },
            "interface": {
                "id": "aaa0e410-2b5b-4815-a80a-a18f2ae59f7d"
            },
            "signature": "BatchPin(address,uint256,string,bytes32,bytes32,string,bytes32[])"
        }
    ],
    "topic": "batch-pin",
    "options": {
        "firstEvent": "oldest"
    }
}

codecov-commenter · 2023-10-12T21:02:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c00ecf8) to head (fc18b63).

Additional details and impacted files

@@            Coverage Diff             @@
##              main     #1418    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          324       325     +1     
  Lines        23706     24019   +313     
==========================================
+ Hits         23706     24019   +313

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

peterbroadhurst · 2024-01-13T17:46:15Z

which we used to do, in order to check for duplicates. I'm not sure if this is required or not, but wanted to call it out.

@nguyer (fyi @EnriqueL8) - there is a really good reason why we need this (at least prior to this PR). That's because calculating the signature of a listener is not an easy thing to do - and FireFly doesn't expose any way for code outside of FireFly to do it.

So if you think about the common startup reconcile in most applications, where it does a query-then-create on the listeners it needs, you need to be able to match the things you get back, to the things you need.

That means you need to be able to create the signature for the things you need (not just the things you create).

Currently, we avoid this by having it so that if you try and recreate the new thing with the same signature, it prevents you with a 409. In fact that's important enough that we found and fixed a bug in it recently in #1433.

Now I can imaging this PR might provide a proposal for a fundamentally new way to do this, by pushing the complex reconcile logic inside of listener. But we do need a clear migration path, and to know what the impact would be on apps relying on the old behavior - because without any thought they would duplicate a new listener on every startup.

nguyer · 2024-02-13T20:13:13Z

@peterbroadhurst I'm trying to pick this back up but really not sure what direction to take it. I think part of my struggle is that I myself haven't run into the use case that is driving the need for this functionality, so I'm sort of designing a solution to an unknown problem.

I could add an updated version of the constraint we used to have (and maybe do some heavy DB schema changes to support that) but what would the constraint be? Topic/Location/[SOMETHING]. Is that last element of the constraint the combined signature of each filter in the listener? Do we need to prevent someone from creating the same exact Topic/Location/[Combination of Signatures] or do we want to prevent the same Topic/Location/[Single event signature] from being used in multiple listeners even if the set of filters in each of those listeners is different. Does that question make sense?

I feel like this PR probably needs another round of design discussion before it's ready for more coding.

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 · 2024-06-07T17:30:36Z

New approach is that we have introduced a new API at /contract/listeners/hash that accepts the same payload as a POST. I did think we could have a subset of payload so just the filters and event part, could clean that up but for consistency kept the same. This API will either return the FilterHash for the filters or the deprecated signature for one event and the filters hash as well. Exposes as well the filterHash to be returned from the POST when creating a new listeners and queryable on the GET of listeners to figure out.

PR needs some conflict cleaning and test coverage but test in a FF CLI setup and looking good!

peterbroadhurst

Hey @EnriqueL8 - lots of mini comments here, but it all rolls up to one big question about the relationship between:

len(filters) == 1 so signature is like SomeEvent(1,2,3)
len(filters) > 1 so:
- Each filter has a signature like like SomeEventA(1,2,3) etc. is queryable in the filter[0] JSON
- The signature of the listener is hash(filters[*].signature)

Then the implication of this is that signature + topic is still a uniqueness criteria on the listener (assuming it is at 1.3.0)

.vscode/launch.json

db/migrations/postgres/000117_update_contractlisteners_add_filters_column.up.sql

internal/apiserver/route_post_contract_listeners_hash.go

peterbroadhurst · 2024-06-07T17:52:05Z

internal/blockchain/ethereum/ethereum.go

+
+	filters := make([]*filter, 0)
+	// This keeps the existing behaviour where a contract listener could only listen to one event
+	if listener.Event != nil {


Feels odd to me that it's the blockchain/ethereum layer responsible for this array-ification.
Why isn't this done for us in a consistent deprecated field handler, in the FF/API layer, before calling this always with an array.

If then a blockchain connector supports only len(filters) == 1, it can return an error

Yeah makes sense - I think this was done before for reload of contract listeners on restart, but how not standardise that on load of listeners from DB instead of here!

Need to address this now

internal/contracts/manager.go

peterbroadhurst · 2024-06-07T17:57:35Z

internal/contracts/manager.go

@@ -835,6 +901,50 @@ func (cm *contractManager) AddContractListener(ctx context.Context, listener *co
 		listener.Options.FirstEvent = cm.getDefaultContractListenerOptions().FirstEvent
 	}

+	// Normalize Event/EventPath + Location to list of Listeners
+	if len(listener.Filters) == 0 {
+		listener.Filters = append(listener.Filters, &core.ListenerFilterInput{


This is what I was expecting to see as the migration from the deprecated fields.
The fact this is in the manager level, makes me confused why any other code apart from this one bit needs to worry about the fact that "on the API boundary we have a special spelling for handling for len(filters) == 1"

Have added this at the manager level and also still provided the deprecated fields in the API

Address this in the blockchain plugin

internal/contracts/manager.go

pkg/core/contract_listener.go

EnriqueL8 · 2024-06-07T18:22:02Z

For the second case, we need to include the location if not it will not be unique and will cause problems

EnriqueL8 · 2024-06-11T12:57:43Z

@peterbroadhurst Thanks for the comments - will amend the PR with changed.

I think the challenge here is the location:

The current uniqueness constraints is location + topic + signature which makes sense
I think we need to keep the same thing at top level so what I propose is that for:
- len(filters) == 1 we stay with location + topic + signature.
- len(filters) > 1 we embed all the possible different locations into the hash that becomes the signature and still keep the signature for each event in so queryable in the filter[0] JSON. The signature then becomes hash(filters[*].signature+filters[*].location)

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 · 2024-06-12T13:33:58Z

@peterbroadhurst From the review comments and the chances I made, came up with three other worries:

Filtering over certain fields at the API level will be worse as we are nesting a lot of the fields such as location, eventPath, etc.. under a JSONAny that is stored as TEXT in one column and doesn't have the native filtering. Wondering how bad that would be?
Have made an opinionated assumption that both Tezos and Fabric only support a single filter for their event streams. I'm pretty certain that Fabric does restrict you to one, @denisandreenko I wonder if you might know if tezosconnect supports multiple filters for the subscriptions?
Multiple filters that clash with each other... so probably need to check that

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

denisandreenko · 2024-06-12T14:44:51Z

@EnriqueL8 tezosconnect doesn't support event streaming yet, so don't worry about compatibility with this connector

EnriqueL8 · 2024-06-12T14:46:24Z

@denisandreenko thanks for the input - so a follow up PR might be needed to provide the user with a better error on event streaming because it seems the code does go ahead and try and create a subscription in the Tezos connector

EnriqueL8 · 2024-06-12T14:53:08Z

Looks like Docker has been update in the Github Action and the packaged docker-compose version no longer support ContainerConfig docker/compose#11742 (comment)

EnriqueL8 · 2024-06-12T14:58:19Z

The GH action is using V1 for some reason /usr/local/bin/docker-compose up -d and it should be using v2. FireFly CLI checks if V1 is available and uses that or V2 if available

EnriqueL8 · 2024-06-13T16:35:03Z

This should do the trick hyperledger/firefly-cli#307 - I'll have to update the action to pull in a new version

peterbroadhurst · 2024-06-17T20:27:48Z

Multiple filters that clash with each other... so probably need to check that

This comment you made is highlighted, but there's no commentary on where you got to with it @EnriqueL8 .
What was your analysis/proposal?

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 · 2024-07-08T15:55:39Z

internal/contracts/manager.go

+			// comparing to channel + chaincode
+			if strings.Contains(duplicateMapChecker[filter.Signature], location) || strings.Contains(location, duplicateMapChecker[filter.Signature]) {
+				return i18n.NewError(ctx, coremsgs.MsgDuplicateContractListenerFilterLocation)
+			}
+


I do wonder if this should be moved to the plugin specific code... or the plugin should generate a signature including the location

What's your proposal here @EnriqueL8? Are you suggesting you make a change before this goes in?

I think your point that requiring location to be part of the signature is valid - but you'd be best positioned to comment on the cost vs. benefit in the code I think.

EnriqueL8 · 2024-07-08T15:57:20Z

internal/contracts/manager.go

+				for _, listener := range existing {
+					// We have extended the event signature to add more information
+					// So we compare the start with is not guaranteed to be the same
+					// but it's the best comparison
+					if strings.HasPrefix(signature, listener.Signature) {
+						return i18n.NewError(ctx, coremsgs.MsgContractListenerExists)


Again maybe this comparing of signatures should be plugin specific

I don't think comparing signatures should be specific, but I don't understand the HasPrefix here.
Surely the signatures are the same, or they are not.

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

doc-site/docs/reference/types/contractlistener.md

internal/blockchain/fabric/fabric.go

internal/contracts/manager.go

peterbroadhurst

Next round of detail comments/questions up for your @EnriqueL8 - these are all around the "how" I think at this point, so that's great.

Pausing here as I do think there's some significant questions here for you to think about before I go further.

db/migrations/postgres/000117_update_contractlisteners_add_filters_column.up.sql

EnriqueL8 · 2024-07-09T12:57:01Z

@peterbroadhurst Just working through the changes came across apis/{apiName}/listeners/{eventPath} which uses the signature to query the DB for the listener... We need to make a decision if this API now makes sense with multiple filters in one listeners and this hierarchy only makes sense for one event or we implement a logic to look in the filters and find out if the event exists

EnriqueL8 · 2024-07-09T13:00:51Z

I think it's quite easy to sort it by checking if the whole signature contains the current event signature and location!

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 · 2024-07-11T12:58:07Z

Great code coverage is working, fixing

Weird I'm getting 100% locally for coverage

internal/blockchain/ethereum/ethereum.go

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

Co-authored-by: Peter Broadhurst <peter.broadhurst@kaleido.io> Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 · 2024-07-11T14:19:10Z

The Postgres migration seem to be failing, looking into it.. It's a shame the GitHub actions do no give you logs of the docker containers, something to add to my list

EnriqueL8 · 2024-07-11T14:20:38Z

Replicate locally FF00184: Database migration failed: FF00184: Database migration failed: migration failed: syntax error at or near "MAX" (column 67) in line 4: BEGIN;

Changing it to just VARCHAR which according to the Postgres documentation is unbounded https://www.postgresql.org/docs/current/datatype-character.html
If character varying (or varchar) is used without length specifier, the type accepts strings of any length

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

peterbroadhurst

✨ 🚀 🌑

nguyer added the migration_consideration label Oct 12, 2023

nguyer requested a review from a team as a code owner October 12, 2023 20:56

nguyer marked this pull request as draft November 8, 2023 16:44

nguyer marked this pull request as ready for review February 13, 2024 15:25

nguyer added 3 commits March 5, 2024 16:21

Enable contract listeners with multiple filters

25c32e3

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

WIP: More work on multiple filters

135cfb0

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

Fixes for multiple filters

7f233a0

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

nguyer force-pushed the filters branch from 9d14273 to 7f233a0 Compare March 6, 2024 16:28

New contract listener Hash API

5a73d6f

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

peterbroadhurst reviewed Jun 7, 2024

View reviewed changes

EnriqueL8 added 5 commits June 12, 2024 10:55

Review comments

ad9cca2

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

revert this

0ddb3d1

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

More review comments

e5b91e7

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

check filters in blockchain plugins

d004e15

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

more fixes and test

230d707

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 added 2 commits June 12, 2024 14:52

Merge remote-tracking branch 'origin/main' into filters

3cb7f78

fix mocks and tests

1d37c4c

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 added 2 commits July 4, 2024 16:14

fix PSQL migration

8320273

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

fix ethconnect subscriptions

26efff4

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 reviewed Jul 8, 2024

View reviewed changes

EnriqueL8 added 2 commits July 8, 2024 17:02

a few more tweaks

66436d3

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

finally got to 100% coverage

4ac0235

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

doc-site/docs/reference/types/contractlistener.md Outdated Show resolved Hide resolved

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

internal/blockchain/fabric/fabric.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

internal/contracts/manager.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

internal/contracts/manager.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

internal/contracts/manager.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed Jul 8, 2024

View reviewed changes

db/migrations/postgres/000117_update_contractlisteners_add_filters_column.up.sql Show resolved Hide resolved

EnriqueL8 added 3 commits July 11, 2024 13:08

Move the signature with location to the blockchain plugins

12e018b

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

Merge remote-tracking branch 'origin/main' into filters

d6cfa8f

fix test after conflicts

e43809c

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

peterbroadhurst reviewed Jul 11, 2024

View reviewed changes

internal/blockchain/ethereum/ethereum.go Outdated Show resolved Hide resolved

EnriqueL8 and others added 3 commits July 11, 2024 14:19

fix coverage for sqlcommon

c85c247

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

Update internal/blockchain/ethereum/ethereum.go

9777588

Co-authored-by: Peter Broadhurst <peter.broadhurst@kaleido.io> Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

fix linting

a468891

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

move to VARCHAR for postgres

fc18b63

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

EnriqueL8 mentioned this pull request Jul 11, 2024

v1.3.1 Test Plan #1538

Closed

peterbroadhurst approved these changes Jul 15, 2024

View reviewed changes

peterbroadhurst merged commit a56d9e8 into hyperledger:main Jul 15, 2024
16 checks passed

peterbroadhurst deleted the filters branch July 15, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable contract listeners with multiple filters #1418

Enable contract listeners with multiple filters #1418

nguyer commented Oct 12, 2023 •

edited

Loading

codecov-commenter commented Oct 12, 2023 •

edited by codecov bot

Loading

peterbroadhurst commented Jan 13, 2024

nguyer commented Feb 13, 2024 •

edited

Loading

EnriqueL8 commented Jun 7, 2024

peterbroadhurst left a comment •

edited

Loading

peterbroadhurst Jun 7, 2024

peterbroadhurst Jun 7, 2024

EnriqueL8 Jun 12, 2024

EnriqueL8 Jul 4, 2024

peterbroadhurst Jun 7, 2024

EnriqueL8 Jun 12, 2024

EnriqueL8 Jul 4, 2024

EnriqueL8 commented Jun 7, 2024

EnriqueL8 commented Jun 11, 2024 •

edited

Loading

EnriqueL8 commented Jun 12, 2024 •

edited

Loading

denisandreenko commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 13, 2024 •

edited

Loading

peterbroadhurst commented Jun 17, 2024

EnriqueL8 Jul 8, 2024

peterbroadhurst Jul 8, 2024 •

edited

Loading

EnriqueL8 Jul 8, 2024

peterbroadhurst Jul 8, 2024

peterbroadhurst left a comment

EnriqueL8 commented Jul 9, 2024 •

edited

Loading

EnriqueL8 commented Jul 9, 2024

EnriqueL8 commented Jul 11, 2024 •

edited

Loading

EnriqueL8 commented Jul 11, 2024

EnriqueL8 commented Jul 11, 2024 •

edited

Loading

peterbroadhurst left a comment

Enable contract listeners with multiple filters #1418

Enable contract listeners with multiple filters #1418

Conversation

nguyer commented Oct 12, 2023 • edited Loading

Open questions

Example

Create contract listener request

Create contract listener response

codecov-commenter commented Oct 12, 2023 • edited by codecov bot Loading

Codecov Report

peterbroadhurst commented Jan 13, 2024

nguyer commented Feb 13, 2024 • edited Loading

EnriqueL8 commented Jun 7, 2024

peterbroadhurst left a comment • 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

Choose a reason for hiding this comment

EnriqueL8 commented Jun 7, 2024

EnriqueL8 commented Jun 11, 2024 • edited Loading

EnriqueL8 commented Jun 12, 2024 • edited Loading

denisandreenko commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 12, 2024

EnriqueL8 commented Jun 13, 2024 • edited Loading

peterbroadhurst commented Jun 17, 2024

Choose a reason for hiding this comment

peterbroadhurst Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

EnriqueL8 commented Jul 9, 2024 • edited Loading

EnriqueL8 commented Jul 9, 2024

EnriqueL8 commented Jul 11, 2024 • edited Loading

EnriqueL8 commented Jul 11, 2024

EnriqueL8 commented Jul 11, 2024 • edited Loading

peterbroadhurst left a comment

Choose a reason for hiding this comment

nguyer commented Oct 12, 2023 •

edited

Loading

codecov-commenter commented Oct 12, 2023 •

edited by codecov bot

Loading

nguyer commented Feb 13, 2024 •

edited

Loading

peterbroadhurst left a comment •

edited

Loading

EnriqueL8 commented Jun 11, 2024 •

edited

Loading

EnriqueL8 commented Jun 12, 2024 •

edited

Loading

EnriqueL8 commented Jun 13, 2024 •

edited

Loading

peterbroadhurst Jul 8, 2024 •

edited

Loading

EnriqueL8 commented Jul 9, 2024 •

edited

Loading

EnriqueL8 commented Jul 11, 2024 •

edited

Loading

EnriqueL8 commented Jul 11, 2024 •

edited

Loading