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/scroll rpc #136

Merged
merged 95 commits into from
Jan 22, 2025
Merged

Feat/scroll rpc #136

merged 95 commits into from
Jan 22, 2025

Conversation

georgehao
Copy link
Member

@georgehao georgehao commented Jan 14, 2025 •
edited
Loading

Description

Because the scroll l2geth adds extra fields (l1Fee, queueIndex) to Transaction, Receipt, and also imports a new transaction type TxL1Message to deal with the deposit message, so scroll-reth also needs to implement scroll-based api like l2geth.

Some affected APIs contain:

  • eth_getBlockReceipts
  • eth_getTransactionReceipt
  • eth_getTransactionByBlockHashAndIndex
  • eth_getTransactionByHash
  • ...

ScrollEthApi is defined here

the scroll-based apis mainly live in:

greged93
greged93 reviewed Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@greged93 greged93 left a comment

Choose a reason for hiding this comment

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

Please remove the #[allow(clippy::missing_const_for_fn)] where possible and replace with const. Just a couple of nits otherwise

crates/scroll/alloy/consensus/README.md Outdated Show resolved Hide resolved
crates/scroll/alloy/rpc-types/src/receipt.rs Outdated Show resolved Hide resolved
crates/scroll/rpc/src/eth/call.rs Outdated Show resolved Hide resolved
@georgehao
Copy link
Member Author

georgehao commented Jan 21, 2025 •
edited
Loading

Please remove the #[allow(clippy::missing_const_for_fn)] where possible and replace with const. Just a couple of nits otherwise

yeah. I all add const tested for every #[allow(clippy::missing_const_for_fn)] replacement, however add const all lint failed

frisitano
frisitano reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Looks good! I left some additional comments inline.

crates/chain-state/src/in_memory.rs Show resolved Hide resolved
crates/scroll/alloy/consensus/src/transaction/tx_type.rs Outdated Show resolved Hide resolved
crates/scroll/rpc/src/error.rs Outdated Show resolved Hide resolved
crates/scroll/rpc/src/error.rs Outdated Show resolved Hide resolved
crates/scroll/rpc/src/eth/receipt.rs Outdated Show resolved Hide resolved
greged93 and others added 3 commits January 22, 2025 16:24
@greged93
feat: scroll primitives (#142)
02a5317 
* feat: scroll primitives

Signed-off-by: Gregory Edison <[email protected]>

* feat: add missing traits implementations

Signed-off-by: Gregory Edison <[email protected]>

* feat: add `ScrollTransactionReceipt`

Signed-off-by: Gregory Edison <[email protected]>

* fix: lint + tests

Signed-off-by: Gregory Edison <[email protected]>

* fix: lint

Signed-off-by: Gregory Edison <[email protected]>

* fix: answer comments

Signed-off-by: Gregory Edison <[email protected]>

* fix: codespell

Signed-off-by: Gregory Edison <[email protected]>

---------

Signed-off-by: Gregory Edison <[email protected]>
@georgehao
address comments
b4a5db9
@georgehao
Merge branch 'feat/scroll-rpc1' of github.com:scroll-tech/reth into f…
f050c40 
…eat/scroll-rpc1
greged93
greged93 reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@greged93 greged93 left a comment

Choose a reason for hiding this comment

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

lgtm! will leave @frisitano to merge

@frisitano frisitano mentioned this pull request Jan 22, 2025
Open
frisitano
frisitano approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@frisitano frisitano 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! LGTM

@greged93 greged93 merged commit d08f7d0 into scroll Jan 22, 2025
45 checks passed
@greged93 greged93 deleted the feat/scroll-rpc1 branch January 22, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greged93 greged93 greged93 left review comments

@frisitano frisitano frisitano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@georgehao @frisitano @greged93 @Thegaram