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

feat: add rpc trait bindings for optimism specific endpoints #7621

Merged
merged 10 commits into from Apr 16, 2024

Conversation

AbnerZheng
Copy link
Contributor

close #7540

Check whether it is in the right direction.

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.

nice, this is the right direction.

I'm okay with the location for now, but we will move this shortly

crates/rpc/rpc-api/src/optimism.rs Outdated Show resolved Hide resolved
crates/rpc/rpc-api/src/optimism.rs Outdated Show resolved Hide resolved
pub hash: BlockHash,
pub number: BlockNumber,
pub parent_hash: BlockHash,
pub timestamp: U64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

use u64 and serde with alloy::u64_hex

crates/rpc/rpc-api/src/optimism.rs Outdated Show resolved Hide resolved
@mattsse mattsse added the A-rpc Related to the RPC implementation label Apr 14, 2024
@mattsse mattsse marked this pull request as ready for review April 14, 2024 12:46
crates/rpc/rpc-api/Cargo.toml Outdated Show resolved Hide resolved
pub hash: B256,
pub number: BlockNumber,
pub parent_hash: B256,
pub timestamp: U64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should also use primitive u64 with alloy_serde::u64_hex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AbnerZheng AbnerZheng marked this pull request as draft April 15, 2024 11:51
@AbnerZheng
Copy link
Contributor Author

Turn it to a draft PR, since there are still much more APIs and tests to add. Sorry for bothering, and will ping you if ready.

@mattsse
Copy link
Collaborator

mattsse commented Apr 15, 2024

Sorry for bothering, and will ping you if ready.

no worries, ping anytime :)

@AbnerZheng AbnerZheng marked this pull request as ready for review April 15, 2024 15:57
@AbnerZheng
Copy link
Contributor Author

@mattsse It's ready for review now. PTAL when available.

AbnerZheng and others added 2 commits April 16, 2024 00:02
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
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.

nice,

lats nits,

please double check u64 usage which should use alloy_rpc_types::serde::u64_hex if hex

Copy link
Collaborator

Choose a reason for hiding this comment

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

please inline this, I want to move the types sone and have a string with this would be easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Done.

pub chain_id: ChainId,
pub latency: Duration,
pub latency: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this seconds or ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time.duration in go, it should be nanosecond.

Comment on lines 91 to 114
#[serde(skip_serializing_if = "Option::is_none")]
pub l1_chain_id: Option<u128>,
#[serde(skip_serializing_if = "Option::is_none")]
pub l2_chain_id: Option<u128>,

#[serde(skip_serializing_if = "Option::is_none")]
pub regolith_time: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
pub canyon_time: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
pub delta_time: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
pub ecotone_time: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
pub fjord_time: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
pub interop_time: Option<u64>,
pub batch_inbox_address: Address,
pub deposit_contract_address: Address,
pub l1_system_config_address: Address,
#[serde(skip_serializing_if = "Option::is_none")]
pub protocol_versions_address: Option<Address>,
#[serde(skip_serializing_if = "Option::is_none")]
pub da_challenge_address: Option<Address>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

all skips, need default attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pub batcher_addr: Address,
pub overhead: B256,
pub scalar: B256,
pub gas_limit: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this must serialize with hex alloy_rpc_types::serde::u64_hex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated tests and
utilize the result returned from a truly running node in tests, as provided
by QuickNode. You can use

curl https://docs-demo.optimism.quiknode.pro/ \
 -X POST \
 -H "Content-Type: application/json" \
 --data '{"method":"optimism_syncStatus","params":[],"id":1,"jsonrpc":"2.0"}'

and

curl https://docs-demo.optimism.quiknode.pro/ \
 -X POST \
 -H "Content-Type: application/json" \
 --data '{"method":"optimism_rollupConfig","params":[],"id":1,"jsonrpc":"2.0"}'

to get rpc result, in these response, the "timestamp" is 1713235375, gasLimit is 30000000, both of them are not hex encoded.

For more information, refer to the QuickNode documentation
at https://www.quicknode.com/docs/optimism/optimism_rollupConfig.

@@ -27,7 +33,7 @@ pub struct L1BlockRef {
pub hash: B256,
pub number: BlockNumber,
pub parent_hash: B256,
pub timestamp: U64,
pub timestamp: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this also needs
alloy_rpc_types::serde::u64_hex

Utilize the result returned from a trully running node, as provided
by QuickNode. For more information, refer to the QuickNode documentation
at https://www.quicknode.com/docs/optimism/optimism_rollupConfig.
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.

ty!

@mattsse mattsse added this pull request to the merge queue Apr 16, 2024
Merged via the queue into paradigmxyz:main with commit 516e836 Apr 16, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rpc trait bindings for optimism specific endpoints
2 participants