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

transaction: add searched event #80

Closed
wants to merge 2 commits into from
Closed

transaction: add searched event #80

wants to merge 2 commits into from

Conversation

josepot
Copy link
Contributor

@josepot josepot commented Aug 17, 2023

Close #79

cc: @tomaka

@josepot josepot requested a review from lexnv August 17, 2023 08:55
@josepot
Copy link
Contributor Author

josepot commented Aug 17, 2023

I went with an Array for the payload because I thought it might be easier for the server. But honestly, from the user's side, it doesn't make a huge difference to me. Whether we get multiple events one by one or a single event with a list of "searched" blocks, it's all good. If there's a way that works better for the server or makes things smoother, let's roll with that. Thoughts?

@tomaka
Copy link
Contributor

tomaka commented Aug 17, 2023

I haven't given a lot of thoughts to this yet, but three remarks:

  • I'd be strongly in favor of merging this somehow with the other events. It feels very sloppy to have this completely separate even though the logic of this event directly interacts with the logic of other events.
  • The server must be able to drop a transaction if the client doesn't pull events quickly enough. This is actually mildly annoying given that right now the server can only drop a transaction for chain-related reasons and not JSON-RPC-client-related reasons.
  • It should be mentioned that these blocks might not overlap in any way with the blocks of chainHead, in other words that there's zero guarantee.

Also, this needs more modifications than just adding the event. There are multiple places in the spec that describe how transactions watching works, including the DoS resilience chapter.

@josepot
Copy link
Contributor Author

josepot commented Aug 17, 2023

Regarding merging the event with others:
I initially separated the event, thinking it'd cleanly add to the existing structure without messing with the current flow. But I get where you're coming from, and I see the appeal in consolidating the logic.

On the server dropping transactions:
I'm trying to grasp how this new event could cause delays in pulling events. From my client-side perspective, I don't see it becoming an issue. But I can imagine situations where it might be problematic for clients on constrained devices. If that's a concern, maybe we could introduce an optional parameter in transaction_unstable_submitAndWatch like withSearchedEvents, defaulting to false. That way, only interested consumers would receive these events.

Regarding the non-overlapping blocks with chainHead:
Totally missed that. Thanks for pointing it out!

About the larger modifications:
Honestly, I was under the impression that integrating the event this way would simplify things, but I now realize the complexity.

To be frank, I might not be the best person to take this forward. I just gave this PR a shot because I was asked to. Hopefully, this PR serves as a starting point for what consumers like me are looking for.

Given the situation, I think I'll close this PR and hand the reins over to @tomaka. If there's any chance that the withSearchedEvents parameter idea could revive this PR, do let me know, and I'll make the necessary updates. Otherwise, I'll be on standby for your PR, @tomaka. And no pressure to respond to all this – just putting my thoughts out there! 😊

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

Successfully merging this pull request may close these issues.

Coupling between transaction and chainhead functions
2 participants