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

Feature gate jsonrpsee related crates and check features powerset for reth-rpc-types #10141

Merged

Conversation

citizen-stig
Copy link
Contributor

Third frollow up of #10130

jsonrpsee related crates contain several dependencies that cannot be easily compiled with riscv, for instance ring v17.

This PR feature gates all jsonrpsee dependencies behind default feature

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

where does this surface an issue for you?
do you depend on reth-rpc-types directly or some other crate that depends on it?

@citizen-stig
Copy link
Contributor Author

where does this surface an issue for you? do you depend on reth-rpc-types directly or some other crate that depends on it?

Yes, we do depend on this crate for ZK rollups on Sovereign SDK.

Also, original code has "jsonrpsee-types" in default features, so I thought that intent is to not have those types in non-default, which this PR does.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

gotcha, supportive, although this crate acts as a container crate that re-exports stuff, I assume you could also cherry pick only those alloy crates that you need

crates/rpc/rpc-types/Cargo.toml Outdated Show resolved Hide resolved
crates/rpc/rpc-types/Cargo.toml Outdated Show resolved Hide resolved
@mattsse
Copy link
Collaborator

mattsse commented Aug 6, 2024

for instance ring v17.

do you know which one pulls that in?

@citizen-stig
Copy link
Contributor Author

for instance ring v17.

do you know which one pulls that in?

It is via alloy-rpc-types-beacon -> alloy-rpc-types-engine and then jsonwebtoken
Yes, here is in our tree:

│   │   ├── reth-rpc-types v1.0.4 (https://github.com/paradigmxyz/reth?rev=d29c5a88075f8f5e99e844baa34ff60e92e794cb#d29c5a88)
│   │   │   ├── alloy-primitives v0.7.7 (*)
│   │   │   ├── alloy-rpc-types v0.2.1
│   │   │   │   └── ..
│   │   │   ├── alloy-rpc-types-admin v0.2.1
│   │   │   │   └── ..
│   │   │   ├── alloy-rpc-types-anvil v0.2.1
│   │   │   │   └── ..
│   │   │   ├── alloy-rpc-types-beacon v0.2.1
│   │   │   │   ├── alloy-eips v0.2.1 (*)
│   │   │   │   ├── alloy-primitives v0.7.7 (*)
│   │   │   │   ├── alloy-rpc-types-engine v0.2.1
│   │   │   │   │   ├── ...
│   │   │   │   │   ├── jsonwebtoken v9.3.0
│   │   │   │   │   │   ├── base64 v0.21.7
│   │   │   │   │   │   ├── pem v3.0.4
│   │   │   │   │   │   │   └── base64 v0.22.1
│   │   │   │   │   │   ├── ring v0.17.8
...

@citizen-stig citizen-stig force-pushed the sovereign/check-features-rpc-types branch from 9be295f to 724c3d9 Compare August 6, 2024 18:11
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@citizen-stig
Copy link
Contributor Author

gotcha, supportive, although this crate acts as a container crate that re-exports stuff, I assume you could also cherry pick only those alloy crates that you need

Thank you for suggestion, I will explore that.

But this PR will allows us to switch from our fork to original this upstream repo .

Maybe it stretches a little bit, but this crate on main branch is not compiles with default features off.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

naming nitpicks

also, with alloy-rs/alloy#1131 the engine crates should become a non issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

these should also be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, fixed!

crates/rpc/rpc-types/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@mattsse
Copy link
Collaborator

mattsse commented Aug 6, 2024

looks like the cargo hack job found the first issue :)

@mattsse mattsse enabled auto-merge August 6, 2024 22:14
@mattsse mattsse added this pull request to the merge queue Aug 6, 2024
Merged via the queue into paradigmxyz:main with commit 2ab17a4 Aug 6, 2024
33 checks passed
martinezjorge pushed a commit to martinezjorge/reth that referenced this pull request Aug 7, 2024
…or `reth-rpc-types` (paradigmxyz#10141)

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
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.

2 participants