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

support json rpc 2.0 with author_submitAndWatchExtrinsic trusted calls #1623

Merged
merged 25 commits into from
Oct 27, 2024

Conversation

clangenb
Copy link
Contributor

@clangenb clangenb commented Oct 25, 2024

Changes

  • The crucial change was that websocket clients expect that the result of the first RPC response must only contain the subscription ID if it is a subscription, and nothing else. This ID will be used to match subsequent messages to the corresponding subscriptions.
  • This also affected the client a little bit, but as the only send_direct_request use case is author_submitAndWatchExtrinsic currently this could be changed relatively easy. But this change hasn't been integrated holistically enough, we can improve the cli code in some places. Marked as a todo [cli] Refactor sending trusted operations in cli #1625.

Notes:

@clangenb clangenb added A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API labels Oct 25, 2024
@clangenb clangenb changed the title add quick fix to support json rpc 2.0 with author_submitAndWatch trusted calls add quick fix to support json rpc 2.0 with author_submitAndWatchExtrinsic trusted calls Oct 25, 2024
@clangenb clangenb marked this pull request as draft October 25, 2024 15:52
@clangenb clangenb marked this pull request as ready for review October 26, 2024 18:45
Comment on lines +96 to +99
TrustedOperation::direct_call(_) => Err(TrustedOperationError::Default {
msg: "Invalid call to `perform_trusted_operation`, use `send_direct_request` directly"
.into(),
}),
Copy link
Contributor Author

@clangenb clangenb Oct 26, 2024

Choose a reason for hiding this comment

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

I decided to remove this branch here. It is very impractical that all these methods have to return the same type T. Our send_direct_request only ever returned (), which by luck worked as we are trying to decode the RPCResponses value, which was always an empty vector, into it. But this seemed like a massive hack and code smell to me. We should separate the others in the future, see #1625

Now, we just always return the top-hash, so I had to remove it from that branch.

Comment on lines -292 to -294
match receiver.recv() {
Ok(response) => {
debug!("received response");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply remove the nesting, by replacing in match and if let with returning early with ? the behavior was preserved.

@clangenb clangenb requested a review from brenzi October 26, 2024 19:53
@clangenb clangenb changed the title add quick fix to support json rpc 2.0 with author_submitAndWatchExtrinsic trusted calls support json rpc 2.0 with author_submitAndWatchExtrinsic trusted calls Oct 26, 2024
@clangenb
Copy link
Contributor Author

I expect that the CI should work once the storage has been freed, this commit was basically successful: da566a0

Copy link
Collaborator

@brenzi brenzi left a comment

Choose a reason for hiding this comment

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

LGTM

As this breaks pubic api, I suggest to bump the sdk release to 0.15 with this

@clangenb clangenb merged commit a2e0037 into master Oct 27, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants