Skip to content

feat: use single event listener per #91 #120

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.

Sign up for GitHub

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

Open
debuggingfuture wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: use single event listener per #91 #120

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6c658f
add: use single event listener per #91
debuggingfuture Feb 16, 2024
3cc1bc0
fix: use single listener reference
debuggingfuture Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions packages/webapp/src/store/subscriptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

// =================================================================================================

import _ from "lodash"
import { Log } from "viem"
import { getPublicClient } from "wagmi/actions"

Expand Down Expand Up @@ -51,8 +52,8 @@ const eventNames = [
/** ID of the game we are currently subscribed to, or null if we are not subscribed. */
let currentlySubscribedID: bigint|null = null

/** List of function to call to unsubscribe from game updates. */
let unsubFunctions: (() => void)[] = []
/** Function to call to unsubscribe from game updates. */
let unsubscribeEventListener: (() => void) | null = null

// -------------------------------------------------------------------------------------------------

Expand All @@ -68,21 +69,37 @@ export function subscribeToGame(ID: bigint|null) {

if (needsUnsub) {
// remove subscription
unsubFunctions.forEach(unsub => unsub())
unsubFunctions = []
if(unsubscribeEventListener){
unsubscribeEventListener()
unsubscribeEventListener = null;
}
console.log(`unsubscribed from game events for game ID ${currentlySubscribedID}`)
currentlySubscribedID = null
}
if (needsSub) {
currentlySubscribedID = ID
eventNames.forEach(eventName => {
unsubFunctions.push(publicClient.watchContractEvent({
address: deployment.Game,
abi: gameABI,
eventName: eventName as any,
args: { gameID: ID },
onLogs: logs => gameEventListener(eventName, logs)
}))

const eventsABI = gameABI.filter((abi) => abi.type === "event" && eventNames.includes(abi.name));

/**
* Listen to all events in eventNames for the current game ID.
* All of these events must have an indexed gameID argument.
* We must use watchEvent to be able to listen to multiple events at the same time.
* Wagmi does not officially support listening to multiple events with an argument filter,
* and this might break in future updates.
*/
Copy link
Copy Markdown
Member

@norswap norswap Feb 18, 2024

Choose a reason for hiding this comment

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

Let's not link the issue here.

I would write something like "Listen to all events in eventNames for the current game ID. All of these events must have an indexed gameID argument in the first position. We must use watchEvent to be able to listen to multiple events at the same time. Wagmi does not officially support listening to multiple events with an argument filter, and this might break in future updates."

Copy link
Copy Markdown
Member

@norswap norswap Feb 18, 2024

Choose a reason for hiding this comment

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

Were you able to confirm that the filtering by game ID actually worked?

My understanding is that it should be possible on the RPC, as long as the gameID is always in the same position, which is case for us (first argument), so the filter would be [<CardDrawn | CardPlayer | ...>, <gameID>]. But what's not sure is whether wagmi is able to make that translation.

You could verify this by opening two different browsers, create a game in both of them (ONLY create). Then once both are created, make the creator join both of them, and check (via the logs printed in the console) that each browser (1) does receive the PlayerJoined event for his own game, and (2) does not receive the PlayerJoined event for another game.

Copy link
Copy Markdown
Author

@debuggingfuture debuggingfuture Feb 19, 2024

Choose a reason for hiding this comment

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

from the reasoning at #91 (comment)
also looking at impl at
https://github.com/wevm/viem/blob/81ae35284e22164e8f8ac4d3b44f846b83f548f1/src/utils/abi/encodeEventTopics.ts#L104

I think viem is smart enough to match args with ABI, so I think actually "an indexed gameID argument in the first position" is not necessary, but "an indexed gameID argument in Ethereum Log event"

Copy link
Copy Markdown
Member

@norswap norswap Feb 19, 2024

Choose a reason for hiding this comment

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

I'm pretty sure this is a limitation in the RPC calls (see link above for eth_getFilterChanges. Viem needs to make the RPC calls and the whole goal of this is to have a single eth_getFilterChanges call for all events. So if the gameID were in different positions, it would need to make multiple (which I'm pretty sure it doesn't do, otherwise this would be a "supported use case" in Viem.

Anyway, we should confirm the actual behaviour from the app anyway :)

unsubscribeEventListener = publicClient.watchEvent({
address: deployment.Game,
events: eventsABI,
args: { gameID: ID },
onLogs: logs => {
Object.entries(_.groupBy(logs, (log:any) => log.eventName))
.forEach(
([eventName, logs]:[string,any]) => {
gameEventListener(eventName, logs)
}
)
}
})
}
}
Expand Down