feat(rpc): Add ots_ namespace and trait bindings for Otterscan Endpoints by ZePedroResende · Pull Request #3778 · paradigmxyz/reth

ZePedroResende · 2023-07-14T10:24:51Z

Motivation

This tackles the first step for #3726 by adding the namespace and trait bindings for the Otterscan RPC endpoints.

Solution

Add ots_ namespace
Add the Otterscan trait to rpc-api
Add necessary custom types for responses to rpc-types
Add a placeholder implementation of the trait to rpc

Co-Authored-By: Miguel Palhas <mpalhas@gmail.com>

ZePedroResende · 2023-07-14T10:30:35Z

Hey @mattsse !
If you have any feedback we would love to hear about it !

mattsse

sweet!

I left a few suggestions, once addressed we can already include this and tackle the actual impl separately

crates/rpc/rpc/src/otterscan.rs

mattsse · 2023-07-14T11:25:36Z

crates/rpc/rpc/src/otterscan.rs

+#[async_trait]
+impl<Eth> OtterscanServer for OtterscanApi<Eth>
+where
+    Eth: EthApiServer,


this might not be enough to fully support the otterscan API, but it is sufficient for now,

but can reevaluate later, see TraceApiImpl for example

crates/rpc/rpc-api/src/otterscan.rs

gakonst

My main Q/concern here is: What new tables are needed to support these APIs?

Could we scope them very clearly, as well as understand 1) what stages (if any) we need to add to generate these new tables, 2) how much DB overhead they incur?

codecov · 2023-07-15T13:53:27Z

Codecov Report

Merging #3778 (9babe78) into main (314e561) will decrease coverage by 0.03%.
The diff coverage is 1.25%.

Impacted Files	Coverage Δ
crates/rpc/rpc-api/src/otterscan.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc-types/src/otterscan.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc/src/otterscan.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc-builder/src/lib.rs	`65.16% <14.28%> (-0.37%)`	⬇️

... and 10 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`15.55% <0.00%> (+<0.01%)`	⬆️
unit-tests	`64.16% <1.25%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`26.48% <ø> (ø)`
blockchain tree	`83.02% <ø> (ø)`
pipeline	`89.60% <ø> (ø)`
storage (db)	`74.26% <ø> (ø)`
trie	`94.65% <ø> (ø)`
txpool	`46.55% <ø> (+0.56%)`	⬆️
networking	`77.70% <ø> (+0.05%)`	⬆️
rpc	`57.55% <1.25%> (-0.38%)`	⬇️
consensus	`65.61% <ø> (ø)`
revm	`33.73% <ø> (ø)`
payload builder	`6.61% <ø> (ø)`
primitives	`88.07% <ø> (+<0.01%)`	⬆️

naps62 · 2023-07-15T14:51:08Z

👍🏼 moving that discussion to #3726 since it seems more on-topic there

crates/rpc/rpc/src/otterscan.rs

crates/rpc/rpc-builder/src/lib.rs

mattsse

q re types.

this is a great first step,
good to merge @ZePedroResende ?

mattsse · 2023-07-16T09:02:15Z

crates/rpc/rpc-types/src/otterscan.rs

+    block_reward: String,
+    uncle_reward: String,
+    issuance: String,


these are likely U256 I believe?

Great question ! Well to get this one i had to read the Go code ,

type internalIssuance struct { BlockReward string `json:"blockReward,omitempty"` UncleReward string `json:"uncleReward,omitempty"` Issuance string `json:"issuance,omitempty"` }

this is the Go struct , and the values are the result of EncodeBig

ret.BlockReward = hexutil.EncodeBig(minerReward.ToBig()) ret.Issuance = hexutil.EncodeBig(issuance.ToBig()) ... ret.UncleReward = hexutil.EncodeBig(issuance.ToBig())

that from the documentation we get

// EncodeBig encodes bigint as a hex string with 0x prefix.

I took the easy route of just reusing the Go type but i would love to know if it still makes sense to use a U256 and have a custom serializer or if the default U256 serializer behaviour is similar to the EncodeBig output !

EncodeBig encodes bigint as a hex string with 0x prefix.

This is the same behaviour

Great ! I'll change it over to U256 then , thanks for catching this !

naps62 · 2023-07-16T09:25:57Z

crates/rpc/rpc-types/src/otterscan.rs

+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct BlockDetails {
+    block: Block,


I didn't notice this at first, but we need a custom serializer (or an entirely different struct even) for this block

e.g.: ots_getBlockDetails does not want the transactions attribute, and instead wants a transactionCount (inside the Block, not as another BlockDetails attribute, as I had understood initially)

naps62 · 2023-07-16T15:11:52Z

crates/rpc/rpc-types/src/otterscan.rs

+#[serde(rename_all = "camelCase")]
+pub struct TransactionsWithReceipts {
+    txs: Vec<Transaction>,
+    receipts: TransactionReceipt,


this should be a Vec

but more importantly, I'm finding now (while working on anvil) that the response otterscan expects doesn't quite match what their spec says,

e.g. for ots_getBlockTransactions, they apparently expect something like:

{ fullblock: { transactions: [ ... ], transactionCount: xx, ... /* all the other usual fields */ }, receipts: [ ... ] }

which is not what the spec says.

overall, maybe it's better to hold off on these structs and instead add them 1 by 1 as we figure out these inconsistencies
wdyt @mattsse ?

My bad ! Should have doubled checked this.
We really need to write some tests , with some request responses then check that the serializations and deserializations matches up.

naps62 · 2023-07-17T12:30:51Z

crates/rpc/rpc-api/src/otterscan.rs

+    async fn search_transactions_after(
+        &self,
+        address: Address,
+        block_number: U256,


@mattsse do these deserialize from integers in reth?
I had problems doing the equivalent in anvil (foundry-rs/foundry#5414)
Otterscan sends 1, but the deserializer expected only "1". had to switch to u64 over there

they don't only hex strings, blocknumber should be U64, and there's also U64HexOrNumber

I used BlockNumberOrTag that is also used on the engine RCP endpoints is that ok or should we use the explicit u64 ?
If that is ok , i think on our side it's ready to merge.

this looks correct:

https://github.com/ledgerwatch/erigon/blob/ef4ccd47736019132c1967d6921f6c4b014eb515/turbo/jsonrpc/otterscan_api.go#L47-L47

mattsse

lgtm
great start!

Adding ots_ namespace and trait bindings

b0f2e11

Co-Authored-By: Miguel Palhas <mpalhas@gmail.com>

ZePedroResende requested review from Rjected and mattsse as code owners July 14, 2023 10:24

ZePedroResende marked this pull request as draft July 14, 2023 10:28

mattsse requested changes Jul 14, 2023

View reviewed changes

code review

92852f7

ZePedroResende requested a review from mattsse July 14, 2023 14:49

naps62 reviewed Jul 14, 2023

View reviewed changes

crates/rpc/rpc-api/src/otterscan.rs Outdated Show resolved Hide resolved

code review

ab84d33

gakonst reviewed Jul 15, 2023

View reviewed changes

ryanschneider reviewed Jul 15, 2023

View reviewed changes

crates/rpc/rpc/src/otterscan.rs Outdated Show resolved Hide resolved

ryanschneider reviewed Jul 15, 2023

View reviewed changes

crates/rpc/rpc-builder/src/lib.rs Show resolved Hide resolved

code review

fdb04f3

mattsse approved these changes Jul 16, 2023

View reviewed changes

mattsse added the A-rpc Related to the RPC implementation label Jul 16, 2023

mattsse marked this pull request as ready for review July 16, 2023 09:03

naps62 reviewed Jul 16, 2023

View reviewed changes

Missing vec

a39b60a

naps62 reviewed Jul 17, 2023

View reviewed changes

ZePedroResende added 2 commits July 17, 2023 19:25

Types code review

33bc94e

Fix block_number type

6f286f8

mattsse approved these changes Jul 18, 2023

View reviewed changes

mattsse enabled auto-merge July 18, 2023 14:02

Merge branch 'main' into traits

77fe385

mattsse added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

mattsse added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

Merge branch 'main' into traits

9babe78

mattsse enabled auto-merge July 18, 2023 22:44

mattsse added this pull request to the merge queue Jul 18, 2023

Merged via the queue into paradigmxyz:main with commit 170e6f2 Jul 19, 2023

Conversation

ZePedroResende commented Jul 14, 2023

Motivation

Solution

Uh oh!

ZePedroResende commented Jul 14, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattsse Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

naps62 commented Jul 15, 2023

Uh oh!

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naps62 Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naps62 Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZePedroResende Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattsse Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 15, 2023 •

edited

Loading

naps62 Jul 16, 2023 •

edited

Loading

naps62 Jul 17, 2023 •

edited

Loading

ZePedroResende Jul 18, 2023 •

edited

Loading