-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: RPC Versioning #242
Conversation
|
||
#[cfg(test)] | ||
#[async_trait] | ||
impl AddTransactionProvider for TestTransactionProvider { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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")] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, updated 🫡
i like the approach quite a lot though, it's very good 🥳 |
Closed in favor of: Because this branch is coming from our fork the CI doesn't work properly. |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
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:All the implementations are then merged in our
RpcModule
in theversioned_rpc_api
function.Methods will be version-prefixed to avoid collisions, for example:
starknet_V0_7_1_blockNumber
,starknet_V0_8_0_blockNumber
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:
RpcVersion
primitive, like:pub const RPC_VERSION_0_7_1: RpcVersion = RpcVersion([0, 7, 1]);
SUPPORTED_RPC_VERSIONS
array,mc-rpc::versions
module,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!