Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs: ahead of v1.3.1 #1544

Merged
merged 18 commits into from
Aug 5, 2024
Merged

docs: ahead of v1.3.1 #1544

merged 18 commits into from
Aug 5, 2024

Conversation

EnriqueL8
Copy link
Contributor

@EnriqueL8 EnriqueL8 commented Jul 22, 2024

Update release notes for the v1.3.1 release.

v1.3.1 included mostly fixes and the main feature to the FireFly Core is the multiple filter per contract listeners

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

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3a2d40c) to head (c5718bb).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1544   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          325       325           
  Lines        24022     24049   +27     
=========================================
+ Hits         24022     24049   +27     

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

Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@EnriqueL8 EnriqueL8 marked this pull request as ready for review July 30, 2024 18:13
@EnriqueL8 EnriqueL8 requested a review from a team as a code owner July 30, 2024 18:13
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>

What's New:

- Enable contract listeners with multiple filters, old format is persevered for backwards compatibility
Copy link
Contributor

@peterbroadhurst peterbroadhurst Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Enable contract listeners with multiple filters, old format is persevered for backwards compatibility
- Enable contract listeners with multiple filters
- See XXXX for detail

While there's no need for migration, we do need documentation for this feature (not sure we'd describe that as "format" as an aside"
The PR for this is not good reference information, due to the churn inside of there - but there's lots of raw information that could be harvested.

The two main places I think need reference to it is:

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's important we fully document this feature, and the reasons you might want to make changes in the application (even though you are not forced to)

EnriqueL8 and others added 6 commits August 1, 2024 17:00
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar
Copy link
Contributor

awrichar commented Aug 1, 2024

I've proposed some of my own edits here: #1550

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
- a connector-specific signature (generated from the event), which allows you to easily identify and search for the contact listener for an event
- a `topic` which determines the ordered stream that these events are part of

This format is still supported by the API. However, it may not fully guarantee accurate ordering of events coming from multiple listeners, even if they share the same `topic` (such as during cases of blockchain catch-up, or when one listener is created much later than the others).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This format is still supported by the API. However, it may not fully guarantee accurate ordering of events coming from multiple listeners, even if they share the same `topic` (such as during cases of blockchain catch-up, or when one listener is created much later than the others).
For backwards compatibility the `event` and `signature` fields are still supported by the API. They contain the `event` from the first `filters` entry.

I think this is the point being made here? Not sure. Overall this paragraph seems a little confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you've made that point below.

I think this paragraph just needs removing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may not fully guarantee accurate ordering of events coming from multiple listeners

This statement has nothing to do with the API input/output format. I think you've elsewhere made the point how the new flexibility of allowing len(filter) > 1 is a good thing.


In addition to this list of multiple filters, the listener specifies a single `topic` to identify the stream of events. This new feature will allow better management of contract listeners and strong ordering of all of the events your application cares about.

Note: For backwards compatibility, the response from the API will populate top-level `event` and `location` fields with the contents of the first event filter in the array.
Copy link
Contributor

@peterbroadhurst peterbroadhurst Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Note: For backwards compatibility, the response from the API will populate top-level `event` and `location` fields with the contents of the first event filter in the array.
### Signature strings
In order to precisely differentiate between different events within an array of filters and prevent duplication, we require a `signature` that exactly matches a single event.
Ethereum provides a string standard for this of the form `EventName(uint256,bytes)`, but this does not fully describe the event because each top-level parameter can in the ABI definition be marked as `indexed`.
For example, the following two famous Solidity event definition have the same signature, but are serialized differently and thus a listener must define both individually to be able to process them:
- ERC-20 `Transfer`
```solidity
event Transfer(address indexed _from, address indexed _to, uint256 _value)
```
- ERC-721`Transfer`
```solidity
event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId);
```
To handle this and provide uniqueness checking of events in a `filters` array while supporting events with different `indexed` settings on the fields, FireFly in 1.3.1 now uses a modified `signature` formula.
The two above would be expressed:
> TODO: @EnriqueL8 or @awrichar to fill in
### Backwards compatibility
- The response from all query APIs of `listeners` will populate top-level `event` and `location` fields
- The first entry from the `filters` array is duplicated to these fields
- On input to create a new `listener` the `event` and `location` fields are supported
- They function identically to supplying a `filters` array with a single entry
- The `signature` field is preserved at the top level
- The format has been changed as described above
- When multiple filters exist the signature contains multiple signature strings, concatenated with `;`

Validated from

sb.WriteString(";")

As an aside, I came across a confusingly named function on my travels (which has nothing to do with functions, which don't have an indexed bool):
https://github.com/hyperledger/firefly/blob/44ab1c72ffa5e47aa06746492b6bd4e72db7288b/internal/blockchain/ethereum/ethereum.go#L1021C6-L1021C33

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems like ABIMethodToIndexedSignature does double duty by helping to parse both methods and events...

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Additional info and clarification in listener docs
@awrichar awrichar merged commit aeec387 into main Aug 5, 2024
16 checks passed
@awrichar awrichar deleted the docs_v1.3.1 branch August 5, 2024 20:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants