feat: add CType block number support

[ntn-x2]

Fixes https://github.com/KILTprotocol/ticket/issues/2330.

It follows a similar process as to what was done for public credentials, and it indeed also refactor some common code into a utils.ts module within the core package, which is not exported outside.

I cannot be merged before KILTprotocol/kilt-node#440 is merged in the node codebase, but it can be reviewed nonetheless.

Checklist

• Wait for node feat: add block number for CTypes kilt-node#440 to be merged
• Wait for SDK fix: block parsing logic for public credentials #697 to be merged
• Address any conflict after merging fix: block parsing logic for public credentials #697

[arty-name]

Looks reasonable

[rflechtner]

I would argue that CType.fromChain is not following the original convention any more, according to which fromChain functions are decoders. I'd say we rename it and remove the necessity of calling api.query.ctype.ctypes before calling it (as it performs state reads anyway). I'm undecided whether the original fromChain should be kept, which would simply return a block number and ctype owner.

[rflechtner]

This used to be a simple decoder (which frankly I think we didn't really need). Given this huge extension in functionality, I'm in favour of replacing this with something like lookupFromChain, which does what this does but additionally calls api.query.ctype.ctypes internally on the hash extracted from the ctype id.

[ntn-x2]

I also thought about it, and wanted to do the same for the public credentials, since we do something very similar. In my opinion, we should get rid of the need to call api.query or api.call for those places that need to do something else with the result anyway, namely ctypes and public credentials. What do you think?

[rflechtner]

As far as they are comparable, yes, that would be my preference. I know very little about how it's done with the public credentials though

[arty-name]

I agree that the code doesn’t really fit the name fromChain. Trying to recall other cases like this in the SDK, I realized that some places were simplified by moving the complexity into the Rust code of RPC calls. Would it make sense to do the same here?

[rflechtner]

If we have a similar case with the public credentials, I'd agree it would be good to rename/change that as well.
On the issue of rpc calls to move these operations to node client, I'm definitely a fan because the nodes tend to be upgraded on runtime changes, so if anything changes storage wise applications don't need to go through the hassle of trying to update at the right time to avoid breaking

[ntn-x2]

I see your point guys, but I also think that considering changing the approach we use for block-based information retrieval is out of scope of this PR. Definitely having an RPC endpoint that clients can call could be beneficial, but we need to remember that RPC methods and types need to be manually upgraded anyway whenever the client is updated, which I would argue is even less "clear" than a runtime update since there could be nodes running different versions of the client software, and since the polkadot library does not support versioning for RPC calls, we would have the same issue of "forcing" node operators to upgrade anyway.
I agree there could be a better way of retrieving this stuff, but what about we merge this, and discuss an alternative strategy that could be applied to public credentials (not touched here), ctypes, and other things in the future?

[ntn-x2]

In the end, this approach, albeit not the optimal, is still better than no approach, and we are not releasing it right away today.

[rflechtner]

Oh yes, i did not want to say that this would keep us from merging the sdk support as it is right now.

[arty-name]

Do you still want to include the renaming of the function in this PR?

[ntn-x2]

@arty-name For public credentials, we call the methods credentialFromChain and credentialsFromChain, and I remember that we used that because although they come from a runtime API the user should not know that, and still use the same method as elsewhere (could not find a comment for it, must have happened via some other channels). Now I feel I am getting the opposite feedback, where we should chain the fromChain naming because it does not come from chain state? What would be your suggestion?

[arty-name]

When I was looking at these PRs I didn’t properly consider the naming convention and everything looked fine to me. Now @rflechtner has raised this topic, and I can see what he means, while I still don’t have a strong opinion. Then I felt that the point was forgotten about in the discussion, and wanted to clarify whether Raphael still thinks it should be done. I believe it’s his call and I will be fine with both outcomes.

[rflechtner]

Ok summing up to avoid confusion:

• please rename fromChain in this PR to something along the lines of fetchFromChain. Inside this function call api.query.ctype.ctypes on the ctype hash taken from the id.
• if these are comparable, align the behaviour of credential(s)FromChain with the above
• reintroduce a fromChain function on CType.chain.ts which acts as a simple decoder, returning {owner, blockNumber}.
• make a ticket for future investigation of rpcs for locating public credentials & ctypes

[ntn-x2]

@rflechtner I addressed your points 1, 2, and 3 in the commits above. In general, I think that mentioning the use of the api.query function when we have api.call is not a good thing. If it would have been possible, we would have hidden the query stuff altogether and simply allow users to use the runtime APIs. Now we have fromChain functions that are meant to be used in conjunction with the query stuff, when the query stuff only provide a partial picture of the data that the user most likely needs. Then why do we decorate the runtime API? I also feel we are re-wrapping functions inside other functions for ease of use, which I personally think goes against the way the SDK was supposed to be used.
This said, as I am definitely less involved than you and @arty-name in the SDK, I will accommodate any suggestions you make, as I have already done in the commits above, but I wanted to express my point of view.

Regarding point 4, we do have a ticket for it, but I don't think it is actually a scalable approach. Now we are putting more load on the server, which is a single instance, rather than putting the parsing load on the client, which are distributed. This will have to be considered carefully, as if we have, potentially thousands of credentials which we want to filter by, say, attester, the node would have to do that once for each client request, while the client could fetch all the data it needs and perform the filtering on its side.

[rflechtner]

Besides some small issues in the docstrings this looks great!

Concerning your remarks, we can have decoders for runtime calls as well; I don't think these are exclusive to state db queries. The convention we introduced simply says that if a function is called *fromChain() it's a simple decoder that transforms a SCALE encoded value into a JSON-like object.

I'm not entirely sure what you're referring to with the wrapping of functions for convenience? If you were talking about the decoders, I would be happy to not have them at all, but could also see that it may get tricky to understand how certain on-chain data is decoded, especially when using js or where e.g. account ids get transformed to URIs/DIDs. I assume it's not fetchFromChain and its publicCredentials cousin, is it? I'd agree we're moving away from what we originally wanted for the sdk with these, but given their complexity writing a recipe instead is just not an option. That's one of the reasons I was pushing for moving this logic to the node side, next to the side effect that this reduces traffic and may help in minimising breaking changes that applications need to try and keep up with.

Side note: I'm not sure I agree that we must try to minimise load on rpc nodes and treat them as 'servers'; should the rpc node not be part of the client-side setup wherever this is feasible (i.e. outside of mobile & browser environments)?