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

feat: RPC Versioning #242

Closed
wants to merge 15 commits into from

Conversation

akhercha
Copy link
Member

@akhercha akhercha commented Sep 3, 2024

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Feature

What is the current behavior?

Resolves: #NA

What is the new behavior?

Introduction of versions for our RPC API.

Versions are defined in the mc-rpc::versions module, organized as follow:

versions
├── mod.rs
└── v0_7_1
    ├── mod.rs
    ├── api.rs               # contains the API definition of the current version
    └── methods              # contains all the methods implementation
         ├── mod.rs
         ├── read/
         ├── trace/
         └── write/

All the implementations are then merged in our RpcModule in the versioned_rpc_api function.

Methods will be version-prefixed to avoid collisions, for example:

  • starknet_V0_7_1_blockNumber,
  • starknet_V0_8_0_blockNumber
  • etc...

The "routing" is done in the VersionMiddlewareLayer middleware, where depending on the version provided in the route by the request's user, we forward to the right method.

If no version is provided, the latest RPC version available is used, defined in RpcVersion::RPC_VERSION_LATEST.

Does this introduce a breaking change?

No

Possible improvement

I don't like the current process of adding a new version ; it is:

  1. Create a new constant in the RpcVersion primitive, like: pub const RPC_VERSION_0_7_1: RpcVersion = RpcVersion([0, 7, 1]);
  2. Add it in the SUPPORTED_RPC_VERSIONS array,
  3. Add the version API + implementation in the mc-rpc::versions module,
  4. Add the module when merging the API methods:
pub fn versioned_rpc_api(starknet: &Starknet, read: bool, write: bool, trace: bool) -> anyhow::Result<RpcModule<()>> {
    let mut rpc_api = RpcModule::new(());

    merge_rpc_versions!(
        rpc_api, starknet, read, write, trace,
        v0_7_1, // We can add new versions by adding the version module below
        // , v0_8_0 (for example)
    );
// ...

Lot of dependency between objects, the middleware also uses the SUPPORTED_RPC_VERSIONS array for routing.

It is fine though because we don't have new versions everyday, that's an uncommon event.
But happy to hear your suggestions!


#[cfg(test)]
#[async_trait]
impl AddTransactionProvider for TestTransactionProvider {
Copy link
Member

Choose a reason for hiding this comment

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

i pulled in the automock crate in #238 for this kind of thing, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

way better yup!

//! ```rust,ignore
//! #[rpc(server, namespace = "starknet")]
//! pub trait JsonRpcV0_7_1 {
//! #[method(name = "V0_7_1_blockNumber")]
Copy link
Member

Choose a reason for hiding this comment

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

this does not follow the spec unfortunately, we'd like to have a prefix like /v0.7.1/ in the URL instead - otherwise our rpcs won't be compatible with the rest of the ecosystem
that's also what juno and pathfinder do iirc

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in DMs it's okey - it does not change the API since we have a middleware that proxies the requests depending on the version requested.

Though do we prefer to have endpoints prefixed with rpc like -> http://localhost:9944/rpc/v0.7.0 or do we prefer http://localhost:9944/v0.7.0? @antiyro

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized that I'm also not sure if it should be:

  • /rpc/v0.7.0
  • or /rpc/v0_7_0

Happy to change - took the dot notation for now since I saw that suggestion on slack.

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be cleaner: http://localhost:9944/rpc/v0_7_0

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, updated 🫡

@cchudant
Copy link
Member

cchudant commented Sep 3, 2024

i like the approach quite a lot though, it's very good 🥳

@akhercha akhercha marked this pull request as ready for review September 3, 2024 17:59
@akhercha akhercha self-assigned this Sep 5, 2024
@akhercha
Copy link
Member Author

akhercha commented Sep 6, 2024

Closed in favor of:
#247

Because this branch is coming from our fork the CI doesn't work properly.

@akhercha akhercha closed this Sep 6, 2024
@antiyro antiyro mentioned this pull request Sep 6, 2024
@akhercha akhercha deleted the feat/rpc_versions branch September 6, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants