feat: use single event listener per #91#120
feat: use single event listener per #91#120debuggingfuture wants to merge 2 commits into0xFableOrg:masterfrom
Conversation
debuggingfuture
changed the title
norswap
left a comment
norswap
left a comment
There was a problem hiding this comment.
Great going, could you confirm the filtering is actually working as intended?
| * use `watchEvent` (instead of watchContractEvent) which will create filter against gameID | ||
| * this is not expected usage of watchEvent and might break when upgrade | ||
| * also only works if the args (gameID) is expected across all event topics | ||
| */ |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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 :)
|
Sample log starting game (sign the first few txns) with |
|
Sample log starting game after this PR d6c658f |
|
Ok, so the logs from Anvil show that |
|
my original thought aftering reading RPC documentation is same as you -- it's a viem limitation and at rpc level just set correct parmas for Apologies first time I test at the app it worked but probably my anvil wasnt stable / race conditions and I came to incorrect conclusion I'd step back and say the reason we're having this convo is we don't hv a way to write a test for reproducable validation I find the params send to looks like it incorrectly making the criteria as OR for 1st argument anyway desired pattern will be [[E1,E2,E3], gameId] or [[E1,E2,E3], null, gameId] etc with E1,E2,E3 as signatures of event |
There's a really easy way to check this without writing a test, I gave it to you earlier ;)
I maybe should have added that we're logging every event received to the browser console, so this is pretty easy to check. It would also be a lot easier than the test you wrote because there is the potential of issues in the test, meaning the test has to be reviewed too :')
Yes, that's exactly what I reckon, although we just want to send |
|
So I performed the check, and I can observe two things:
EDIT: In particular, I get it in the following scenario:
You'll notice that we do not subscribe to the |
|
I also looked at the JSON payload in the Chrome web tools "Network tab" and here's what I'm getting: This seems to confirm your hunch that Viem is broken here, the code you link does indeed flatmap all the individual arrays together. (Also I have no idea where these null values are coming from ....) To support our use case, it should instead "zip" them together (e.g. But for us, the things should be of form Next steps would be:
|
|
Hey, any updates on this? |
2 participants
Context (Problem, Motivation, Solution)
For #91
to reduce rpc usage with single event listener and rpc filter
Describe Your Changes
Checklist
make checkand fixed resulting issuesTesting