paritytech / json-rpc-interface-spec

30 stars 3 forks source link

transaction: add `searched` event #80

Closed josepot closed 1 year ago

josepot commented 1 year ago

Close #79

cc: @tomaka

josepot commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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! 😊