Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: Added methods to handle new transactions #36

Merged
merged 13 commits into from
May 8, 2024

Conversation

jinoosss
Copy link
Member

Description

This PR adds RPC feature support for new transactional events.

This is what works for this PR.

  • Adds a newTransactionFilter method and creates a filter.
  • Check for changes to the transaction filter with the existing getFilterChanges method.
  • Subscribe to transaction creation events via the newTransactions parameter of the websocket subscribe method.

Examples

Using Transaction filter to get transactional changes

tx-indexer-changes

Subscribe to transaction events with websockets

tx-indexer-sub

@jinoosss
Copy link
Member Author

Could you give me some feedback on supporting RPC functionality for transactional events?

@jinoosss jinoosss marked this pull request as ready for review April 18, 2024 05:50
@jinoosss jinoosss requested a review from a team as a code owner April 18, 2024 05:50
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for adding this missing functionality 🙏

I've left a few comments that should be addressed before merging this out. My primary concern is emitting an event in the fetcher for new txs, when we already emit a block event that contains the txs -> I think we should just stick to the block event, and in the FM construct individual tx events for the tx listeners

Tx filters aren't applied because Apply is never called (the apply logic should happen in UpdateWith). We're also missing handling tx filter options when the user creates a transaction filter

I think we're 80% there, these few things are the blockers 🙏

serve/filters/manager.go Outdated Show resolved Hide resolved
serve/filters/types.go Show resolved Hide resolved
serve/handlers/tx/tx.go Outdated Show resolved Hide resolved
fetch/fetch.go Outdated Show resolved Hide resolved
serve/filters/filter/tx.go Outdated Show resolved Hide resolved
serve/handlers/subs/subs.go Outdated Show resolved Hide resolved
serve/filters/manager.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Not a fan of our actual memory filters, created an issue (#38) to improve them.

They might not be even needed, the events JSON RPC API is using WebSockets, right? So we know when a client disconnects, so we might do the same we are doing on GraphQL API.

@jinoosss
Copy link
Member Author

They might not be even needed, the events JSON RPC API is using WebSockets, right? So we know when a client disconnects, so we might do the same we are doing on GraphQL API.

Yes, it can work like GraphQL's subscription feature.
For now, GraphQL's filter feature works better.

But it's also nice to provide familiar and accessible functionality.
The specification for JSON RPC is familiar to users.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this up 🙏

I apologize for the late reply on this, looks great now 💯

@zivkovicmilos zivkovicmilos merged commit a38eb3b into main May 8, 2024
3 checks passed
@zivkovicmilos zivkovicmilos deleted the feat/add-transaction-subscriptions branch May 8, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants