feat: Separate jsonrpc from rest of codebase, to prevent rpcs to breaking due to refactoring.

[pmnoxx]

We should refactor the code to separate jsonrpc from rest of the code base.

Steps 1: remove usage of serde from crates other than jsonrpc. serde is used mainly for Serialize/Deserialize of messages. serde is used in following creates, simply because jsonrpc needs to serialize to json, to read/write messages to network. By removing serde from those crates, we will be one step closer to cleaning jsonrpc

• Remove serde from near-client-primitives. #5515
• re: Remove unused dependencies for near-network #5508
• removing serde from near-primitives (only uses serde with test_fetures)
• re: Remove usage of serde from near-network-primitives #5546

Step 2:

• reorganize jsonrpc and cleanup api, make sure it contains all types used.

See #5428 (review)

[matklad]

I'd make this even more fine-grained: conceptually, jsonrpc consists of two parts:

• definition of data types and routes that comprise our public, stable API
• implementation of actix-based client and server

It's important to separate data type definitions into a separate crate. That way, if someone wants use the API using a different networking stack (eg, based on blocking IO), they get to re-use definitions of the JSON API iteslf, without brining in tokio and actix ecosystems.

I think the end state here is that we publish near_rpc_types crate which:

• doesn't depend on random nearcore crates
• doesn't depend on any "runtime" crates like actix, hyper, tokio, etc.
• doesn't have a lot of dependencies in general, mostly just serde
• defines just the data types
• has a stable API which we are committed to not break
• versioned properly (eg, 1.0.142, like serde)

My suggested path there:

• pick near-jsonrpc-primitives crate as the one that would become near_rpc_types
• move serde usage from other crates into near-jsonrpc-primitives
• remove extra deps from near-jsonrpc-primitives
• rename the crate to have a short, catchy public name (nrpc_types? The call site would be nrpc_types::Block, etc)
• publish 0.1 to crates.io
• audit API very very carefully and publish 1.0 with the commitment to not make 2.0 for at least couple of years.

[matklad]

Also, from me personally: this is a hugely important bit of work which has multiplicative implications for both how effective are folks working at nearcore itself, as well as how robust is the ecosystem which builds on top of us. Kudos for spearheading the effort @pmnoxx !

[matklad]

cc @ChaoticTempest as one of the consures of RPC which cares a lot about Rust-level semver stability.

[frol]

removing serde from near-primitives

@pmnoxx That might be not that clear of a win since actually some other projects might use and serialize those. Given that we publish near-primitives now with a 0.x semver-breaking releases, it is fine if we introduce breaking change since users will still be able to use whatever version they used before and rarely they actually need to upgrade near-primitives.

I think the end state here is that we publish near_rpc_types crate...

Well, near-jsonrpc-primitives is exactly that crate, and indeed it should not depend on other nearcore crates by default and only optionally depend on nearcore crates to implement From trait for those.

[frol]

@miraclx FYI, I had a chat with @matklad and suggested that he collaborate with you on this near-jsonrpc-primitives refactoring. This has been on my radar for quite a while, and I feel we can make these small steps toward better granularity of nearcore project and finally get to the point where we are comfortable releasing near-jsonrpc-client-rs 1.0

[matklad]

Had a design discussion with @miraclx, the write up is here:

https://hackmd.io/WeCthH5LQqGrHiO1dxv6WA

The most important bit is next steps:

• Flatten view types into jsonrpc-primitive types
• Actively remove serde Serilialize/Deserialize impls from near-primitives
• Move serializable types from near-primitives to near-jsonrpc-primitives

Not that "remove nearcore dependencies from jsonrpc-primitives" is not there -- we can do that later, after we just concentrate serialize impls there.

[pmnoxx]

removing serde from near-primitives

@pmnoxx That might be not that clear of a win since actually some other projects might use and serialize those. Given that we publish near-primitives now with a 0.x semver-breaking releases, it is fine if we introduce breaking change since users will still be able to use whatever version they used before and rarely they actually need to upgrade near-primitives.

I think the end state here is that we publish near_rpc_types crate...

Well, near-jsonrpc-primitives is exactly that crate, and indeed it should not depend on other nearcore crates by default and only optionally depend on nearcore crates to implement From trait for those.

To clarify, removing serde from near-primitives is not done for the purpose of improving compile time. As you noticed It's going to be needed to compile dependencies. However. those libraries use serde just, because jsonrpc needs them for purpose of doing message serialization/deserialization of rpc queries and responses. Removing of serde::Serialize / serde::Deserialize shows us that the types in those creates are not used by jsonrpc anymore.

[pmnoxx]

@matklad @frol @miraclx Ok, the part 1 is done. near-network only uses serde with test_features, which doesn't happen in production. near-network-primitives / near-client-primitives doesn't use serde.

There may be some other crates that do.

[matklad]

Some extra observation about view types here: #6068 (comment). TL;DR at least one *View type is stored in the database, which feels suspect:

nearcore/core/store/src/db.rs

Lines 160 to 164 in dd8b28a

	/// `LightClientBlock`s corresponding to the last final block of each completed epoch.
	/// - *Rows*: EpochId (CryptoHash)
	/// - *Content type*: LightClientBlockView
	ColEpochLightClientBlocks = 26,
	/// Mapping from Receipt id to Shard id

[bowenwang1996]

Some extra observation about view types here: #6068 (comment). TL;DR at least one *View type is stored in the database, which feels suspect:

nearcore/core/store/src/db.rs

Lines 160 to 164 in dd8b28a

	/// `LightClientBlock`s corresponding to the last final block of each completed epoch.
	/// - *Rows*: EpochId (CryptoHash)
	/// - *Content type*: LightClientBlockView
	ColEpochLightClientBlocks = 26,
	/// Mapping from Receipt id to Shard id

@matklad #4159 :(

[frol]

This was not fully addressed, see #6587 (comment)

[matklad]

Another approach we should consider is defining the API in language-agnostic way and generating Rust-side of the API from the spec, rather than defining the spec in terms of rust impl.

Here's a good example:

https://www.twilio.com/docs/openapi/generating-a-rust-client-for-twilios-api#setup

openapi-generator generate -g rust \
  -i https://raw.githubusercontent.com/twilio/twilio-oai/main/spec/json/twilio_api_v2010.json \
  -o twilio-rust \

This pulls language-agnostic JSON API spec from github and generates a bunch Rust structs with serde from it.

This is going to be very labor intensive (we'd have to write the API spec essentially from scratch, reverse-engineering from current Rust structs), but should give us (and 3rd party developers!!) the best results. This also probably won't be incremental with respect to tactical refactors suggested above, so, if we do want to pursue this approach at some point, it makes sense to start that today.

I am not familiar with the API specification languages landscape, so I can't vouch for openapi in particular, though I have to admit that the above one-command snippet looks great!

I ... don't really understand why we didn't cover openapi-like approaches in https://hackmd.io/WeCthH5LQqGrHiO1dxv6WA

[frol]

OpenAPI gravitates towards REST-like APIs and it does not support sum-types (enums). We would need to redesign the API in order to make it OpenAPI-friendly (and developers-friendly as well), and while it has been a long-standing wish, we never had time to sit down and address it.