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.

Sign up for GitHub

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

Jump to bottom

feat: integrate request validation in EnginveValidator #13858

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: integrate request validation in EnginveValidator #13858

wants to merge 10 commits into from
+47 −29

Conversation

hoank101
Copy link
Contributor

Closes #13849

emhane
emhane reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

so far so good!

crates/engine/primitives/src/lib.rs Outdated Show resolved Hide resolved
@hoank101 hoank101 marked this pull request as ready for review January 22, 2025 14:09
@hoank101 hoank101 requested a review from emhane January 22, 2025 14:09
emhane
emhane requested changes Jan 22, 2025
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice progress

crates/rpc/rpc-engine-api/src/engine_api.rs Show resolved Hide resolved
@hoank101 hoank101 requested a review from emhane January 22, 2025 14:28
@hoank101
Copy link
Contributor Author

thank @emhane but i don't know why lint/wasm error

@emhane
Copy link
Member

emhane commented Jan 22, 2025

thank @emhane but i don't know why lint/wasm error

some crates don't have no std support, making a pr to fix this. not sure exactly why this was triggered now and not earlier.

@hoank101
Copy link
Contributor Author

some crates don't have no std support, making a pr to fix this. not sure exactly why this was triggered now and not earlier.

thank, please help me review this PR, i think it ok

@emhane
Copy link
Member

emhane commented Jan 22, 2025

@hoank101 try run zepter run, you will need to install the tool if you don't have it. possibly solves problem already.

@emhane emhane added A-consensus Related to the consensus engine A-rpc Related to the RPC implementation labels Jan 22, 2025
emhane
emhane approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

great job!

Rjected
Rjected requested changes Jan 22, 2025
View reviewed changes
crates/engine/primitives/Cargo.toml Outdated Show resolved Hide resolved
@emhane
Copy link
Member

emhane commented Jan 22, 2025

likely blocked by #13922, #13924

@hoank101 hoank101 requested a review from Rjected January 23, 2025 00:44
Rjected
Rjected reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, I have just one nit, otherwise this looks good to me

crates/engine/primitives/src/lib.rs Outdated Show resolved Hide resolved
@hoank101 hoank101 requested a review from Rjected January 23, 2025 04:33
Rjected
Rjected approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emhane emhane emhane approved these changes

@Rjected Rjected Rjected approved these changes

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Assignees
No one assigned
Labels
A-consensus Related to the consensus engine A-rpc Related to the RPC implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate request validation in EngineValidator
3 participants
@hoank101 @emhane @Rjected