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

Implementing a code generator for starknet-types-rpc #9

Merged
merged 28 commits into from
Nov 7, 2023

Conversation

nils-mathieu
Copy link
Contributor

@nils-mathieu nils-mathieu commented Oct 23, 2023

This pull request introduces a code generator (hosted in /crates/starknet-types-rpc/generator) that parses OpenRPC documents provided by Starkware (link) in order to automatically produce the required types.

I'm not sure whether you guys had something in mind for this, but either way I can help. I'm putting this draft PR here so you can check it out and determine whether this is of interest or not.

I was writing the generator anyway for my own personal project (specifically because none of the solutions currently available on <crates.io> perfectly fit my needs.

Pull Request type

This is obviously a new feature :)

What is the current behavior?

The crate is empty and exists as a placeholder.

What is the new behavior?

The crate is no longer empty and contains type definitions!

Does this introduce a breaking change?

Technically, yes. If someone depends on the crate and has something like:

use starknet_types_rpc::*
use some_other_crate::BlockWithTxHashes;

They will get a compilation error 👀

I hope no one is depending on this because I'm too lazy to write a migration guide.

Steps

  • Allow param types to correctly deserialize from arrays and maps (that's part of the JSON-RPC specification).
  • Add optional support for #![no_std] (currently it's always required).
  • Remove the debug paths from the generated file.

Unresolved Questions

Do we want to create a module per version of the specification? (i.e. v0_1_0, v0_2_0, etc.) It would probably be more correct but it would make the crate a bit harder to use (though it's probably not that bad).

Other information

There obviously is some unresolved questions to be answered. The first one is whether code generation is the right approach for this project in the first place. I personally like writing everything by hand to have the freedom to add as much documentation as I want. I also generally like to handcraft everything. However, the following considerations made me change my mind:

  • With a generator, adapating to API updates and new versions becomes trivial.

  • Adding a feature flag to gate something specific provided by the crate is easy and fast. Easy to maintain as well.

  • Creating submodules for every version of the API is very easy and requires little to no change to the generator.

  • We avoid making mistakes while trying to follow the specification. Having a generator doesn't fix this directly, when one fix in the generator fixes a lot more than one fix on a single thing.

However, I do think there are some drawbacks to this:

  • Using a generator makes it a bit harder for new contributor because they need to learn how it work instead of directly interacting with the types they know.

  • Anything that's not directly in the specification (e.g. external types such as stark-felt::Felt) must be added as a special case in a separate configuration file.

  • We're kinda bound to the spec. It's harder to create custom types and definitions. The data model of JSON is very peculiar and its direct translation to Rust can be a bit weird at times. Getting this right procedurally with a generator is definitely non trivial.

In any case, I'd be down to work on that. Generator or not.

@nils-mathieu nils-mathieu reopened this Oct 29, 2023
@nils-mathieu
Copy link
Contributor Author

I just force-pushed to remove the commits related to the generator implementation.

The reason for that is that I found myself writing a lot of non-starknet related code and I decided to shift the generator to a separate crate that's general to any OpenRPC document. It's available here.

@nils-mathieu
Copy link
Contributor Author

nils-mathieu commented Oct 29, 2023

I'll put this ready for review for now to get some feedback, but I still need to write some tests to ensure that types are properly serialized/deserialized.

@nils-mathieu
Copy link
Contributor Author

Unresolved Questions

Do we want to create a module per version of the specification? (i.e. v0_1_0, v0_2_0, etc.) It would probably be more correct but it would make the crate a bit harder to use (though it's probably not that bad).

@0xLucqs
Copy link
Collaborator

0xLucqs commented Oct 30, 2023

@xJonathanLEI already has a code generator for rpc types https://github.com/xJonathanLEI/starknet-jsonrpc-codegen. Also i think that beerus already tried the openrpc type generation and it wasn't that good (can't remember why though i'll ask). For now my first impression is that it adds a lot of useless types and requires to hold the spec file in the repo which i don't like

@nils-mathieu
Copy link
Contributor Author

nils-mathieu commented Oct 30, 2023

Already has a code generator for rpc types

Yeah, I've talked tiny bit with them about it. He's the one that convinced me that it was better than writing everything by hand 😅.

The main thing was that it allowed him to find a few bugs in node implementations thanks to strict code generation, which I think is a nice thing to have.

For the other points, I discused in the PR post.

I like what they did but there several points:

  1. It uses serde_as and a bunch of other dependencies that I didn't want to bring into the crate.
  2. It misses a few quality-of-life detections that I think are important if anyone needs to maintain the repository (not the generator) later.
  3. It seemed (though that I didn't really try out to see for myself) to lack in flexibility in its configuration.

it adds a lot of useless types and requires to hold the spec file in the repo which i don't like.

What useless types are you talking about? You kinda need to declare all the fields that will be serialized in one way or another, so there can't really be any useless fields.

There's some optimization you can do to reduce boilerplate, such as flattening structs that are used once, or those that only have one field (which are performed by my generator).

There's also a few special-case for BlockId and SyncingStatus because their "rusty" representation can be greatly improved if done by hand.

requires to hold the spec file in the repo

It's true right now because the spec is kinda broken. For example it misses a few "type": "object" that would be needded to follow the JSON-Schema specification. I fixed those two typos in the spec and saved the fixed version in the repo. If that's something we don't want, we can always notify them of the issue.

Or we can just not save it and fix it every time. That's not a big deal it takes about 15 seconds.

@xJonathanLEI
Copy link
Collaborator

My only comment here would be that the generator should live in a separate repo but I see that's already the case now.

Generally, I like the approach of generating model code, which is why I did it for starknet-rs. I do also agree that serde_as is suboptimal and I plan to get rid of it at some point (nothing planned at the moment though).

However, I skimmed through the code and there doesn't seem to be any support for query-only version? It's not actually part of the specs so it makes sense that the directly generated code doesn't support it yet. That said, it's widely used in practice and need to be supported for the code to be useful.

@0xLucqs
Copy link
Collaborator

0xLucqs commented Nov 6, 2023

What's the status here ? Waiting for review ?

@nils-mathieu
Copy link
Contributor Author

What's the status here ? Waiting for review ?

Yes!

@0xLucqs
Copy link
Collaborator

0xLucqs commented Nov 6, 2023

ok after you fix all the no std imports i'll review

Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Mini review, can you add comments in your code plz, this repo will be the base for the whole ecosystem, i expect that a lot of people will read this code so can you add some detailed comments on what you're doing inside each function it'd be very helpful

crates/starknet-types-rpc/src/custom_serde.rs Outdated Show resolved Hide resolved
crates/starknet-types-rpc/src/custom_serde.rs Outdated Show resolved Hide resolved
@nils-mathieu
Copy link
Contributor Author

Mini review, can you add comments in your code plz, this repo will be the base for the whole ecosystem, i expect that a lot of people will read this code so can you add some detailed comments on what you're doing inside each function it'd be very helpful

Done! I've added a whole bunch of comments to describe how the algorithm works, as well as some further optimizations on the deserialization side.

@0xLucqs 0xLucqs merged commit 01b222e into starknet-io:main Nov 7, 2023
2 checks passed
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.

4 participants