Skip to content

Conversation

@ntn-x2
Copy link
Contributor

@ntn-x2 ntn-x2 commented Oct 12, 2022

This PR follows the direction that Parity is taking of removing RPC calls that do nothing but forwarding it to the runtime. Plus, RPC calls do not support versioning, while runtime calls do, and this makes it very easy for the SDK to use the right types and functions depending on the runtime API version (tested on a local script using the same type definitions to connect both to Spiritnet and to a node running this PR, and both were successful).

If this PR is merged, #421 becomes less urgent, although we should have a strategy for breaking changes at the RPC level anyway.

How it's used in the SDK

Please refer to the following commit: KILTprotocol/sdk-js@ba1c023.

@ntn-x2 ntn-x2 requested review from weichweich and wischli October 12, 2022 14:13
@ntn-x2 ntn-x2 self-assigned this Oct 12, 2022
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Removal LGTM. However, I think you missed flagging the other DID runtime API call

Already looking forward to rebasing #413 🥲

/// * associated accounts
/// * service endpoints
fn query_did(did: DidIdentifier) -> Option<RawDidLinkedInfo<DidIdentifier, AccountId, LinkableAccountId,Balance, Key, BlockNumber>>;
fn query_did(did: DidIdentifier) -> Option<RawDidLinkedInfo<DidIdentifier, AccountId, LinkableAccountId, Balance, Key, BlockNumber>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have a changed_in flag for this call as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true! I think all of them must be marked as deprecated. Good catch!

@ntn-x2 ntn-x2 enabled auto-merge (squash) October 13, 2022 11:52
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM!

@ntn-x2 ntn-x2 merged commit 2bec676 into develop Oct 13, 2022
@ntn-x2 ntn-x2 deleted the aa/remove-rpc branch October 13, 2022 12:13
wischli pushed a commit that referenced this pull request Oct 17, 2022
* Remove DID RPC

* Add version support for DID RPC call

* Mark other DID API methods as deprecated as well
@wischli wischli mentioned this pull request Oct 18, 2022
5 tasks
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.

3 participants