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.

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

Jump to bottom

rpc module: add call_and_subscribe #588

Merged
merged 8 commits into from
Dec 6, 2021
Merged

rpc module: add call_and_subscribe #588

merged 8 commits into from
Dec 6, 2021

Conversation

niklasad1
Copy link
Member

Needed to support in memory queries in substrate with "optional subscriptions`, example here

This might be useful elsewhere too but I think we should refactor this in v0.7 many similar functions there now.

It won't work unless we expose receiver of the stream which this does.

@niklasad1 niklasad1 requested a review from a team as a code owner December 2, 2021 13:43
dvdplm
dvdplm approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Not 100% sold on this. Couldn't the code in substrate (and cumulus I believe) could be refactored to use test_subscription instead?

Not going to make a fuss, but the API is growing larger than I'd like.

types/src/v2/response.rs Outdated Show resolved Hide resolved
utils/src/server/rpc_module.rs Outdated Show resolved Hide resolved
utils/src/server/rpc_module.rs Outdated Show resolved Hide resolved
utils/src/server/rpc_module.rs Show resolved Hide resolved
@niklasad1
Copy link
Member Author

niklasad1 commented Dec 3, 2021 •
edited
Loading

Not 100% sold on this. Couldn't the code in substrate (and cumulus I believe) could be refactored to use test_subscription instead?

Nah, it would panic if the response wasn't a subscription response but I get your point it's getting messy.

For cumulus and substrate (for testing) it's fine but it's a breaking change for users that actually is using this for subscriptions.
For example paritytech/subxt#341 :P

niklasad1 and others added 5 commits December 3, 2021 10:29
@niklasad1 @dvdplm
Update utils/src/server/rpc_module.rs
46f55b6 
Co-authored-by: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
Update types/src/v2/response.rs
2b09382 
Co-authored-by: David <dvdplm@gmail.com>
@niklasad1
grumbles
ec3c47f
@niklasad1
Merge remote-tracking branch 'origin/na-rpc-module-call-with-subscrip…
d004274 
…tion' into na-rpc-module-call-with-subscription
@niklasad1
fix rustdoc links
11b41c6
@niklasad1
Copy link
Member Author

@dvdplm can you take another look at this?

I think we should remove call in public API, rename call_with to call and perhaps unify the inner the implementation.

dvdplm
dvdplm reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

A few changes to the docs.

utils/src/server/rpc_module.rs Show resolved Hide resolved
utils/src/server/rpc_module.rs Outdated Show resolved Hide resolved
utils/src/server/rpc_module.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Contributor

dvdplm commented Dec 6, 2021

I think we should remove call in public API, rename call_with to call and perhaps unify the inner the implementation.

I agree and I think I tried it but ran into some kind of issue that made me stop trying.

niklasad1 and others added 2 commits December 6, 2021 15:03
@niklasad1 @dvdplm
Update utils/src/server/rpc_module.rs
515d521 
Co-authored-by: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
Update utils/src/server/rpc_module.rs
179857f 
Co-authored-by: David <dvdplm@gmail.com>
@niklasad1 niklasad1 merged commit 3cb5eda into master Dec 6, 2021
@niklasad1 niklasad1 deleted the na-rpc-module-call-with-subscription branch December 6, 2021 14:21
niklasad1 added a commit that referenced this pull request Dec 7, 2021
@niklasad1 @dvdplm
rpc module: add call_and_subscribe (#588)
f8a412e 
* rpc module: add `call_and_subscribe`

* Update utils/src/server/rpc_module.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update types/src/v2/response.rs

Co-authored-by: David <dvdplm@gmail.com>

* grumbles

* fix rustdoc links

* Update utils/src/server/rpc_module.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update utils/src/server/rpc_module.rs

Co-authored-by: David <dvdplm@gmail.com>

Co-authored-by: David <dvdplm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@maciejhirsz maciejhirsz maciejhirsz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@niklasad1 @dvdplm @maciejhirsz