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

refactor(sdk): Support multiple contracts in ChainEventPoller #2928

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Dec 4, 2024

Refactored ChainEventPoller to support multiple contracts simultaneously. Now the class is a singleton and we can define contract methods as a listener parameter. This optimizes log fetching as there needs to be only one poller running even if multiple contract events are listened.

Also we don't now need to create a Contract instance just for adding event a listener. This allows us to simplify e.g. Operator class (in a follow-up PR).

@github-actions github-actions bot added the sdk label Dec 4, 2024
@teogeb teogeb requested a review from harbu December 4, 2024 09:14
@harbu
Copy link
Contributor

harbu commented Dec 4, 2024

To address your point

Added try-catch around the event listener so that the polling doesn't stop if a listener throws an error.

We had a discussion on video meeting (me and Teo ofc), maybe better to leave out and have it as caller's responsibility (especially since it was like this before).

On that front, regarding the async case, maybe eslint rule that giving async callback to place where (sync) callback is expected would be an error?

Comment on lines +90 to +91
address: uniq(this.listeners.map((l) => l.contractAddress)),
topics: [uniq(this.listeners.map((l) => l.contractInterfaceFragment.topicHash))],
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to check that this definitely works as we think it does

}

const createEventLogItem = (
eventName: string, eventArgs: any[], blockNumber: number
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
eventName: string, eventArgs: any[], blockNumber: number
eventName: string,
eventArgs: any[],
blockNumber: number

): Partial<Log> => {
const contractInterface = new Interface(createAbi(eventName))
return {
blockNumber: blockNumber,
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
blockNumber: blockNumber,
blockNumber,

logger.debug('Polling', { fromBlock, eventNames })
events = await contract.queryFilter([eventNames], fromBlock) as EventLog[]
const filter = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth commenting how the filtering logic here works, e.g.

address == (X OR Y OR Z) AND topic === (A OR B OR C)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants