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] Change all Option<BlockId> to BlockId on methods in the Provider #514

Closed
zerosnacks opened this issue Apr 11, 2024 · 4 comments
Closed
Labels
enhancement New feature or request

Comments

@zerosnacks
Copy link
Member

zerosnacks commented Apr 11, 2024

Component

provider, pubsub

Describe the feature you would like

get_code_at and get_uncle on the Provider currently require the user to define the tag whereas others allow for defining tag as None, where None means defaulting to Latest.

/// Gets the bytecode located at the corresponding [Address].
async fn get_code_at(&self, address: Address, tag: BlockId) -> TransportResult<Bytes> {
self.client().request("eth_getCode", (address, tag)).await
}

/// Gets an uncle block through the tag [BlockId] and index [U64].
async fn get_uncle(&self, tag: BlockId, idx: U64) -> TransportResult<Option<Block>> {
match tag {
BlockId::Hash(hash) => {
self.client().request("eth_getUncleByBlockHashAndIndex", (hash, idx)).await
}
BlockId::Number(number) => {
self.client().request("eth_getUncleByBlockNumberAndIndex", (number, idx)).await
}
}
}

Whereas other methods allow for defining tag as None,

/// Gets the transaction count (AKA "nonce") of the corresponding address.
#[doc(alias = "get_nonce")]
#[doc(alias = "get_account_nonce")]
async fn get_transaction_count(
&self,
address: Address,
tag: Option<BlockId>,
) -> TransportResult<u64> {
self.client()
.request("eth_getTransactionCount", (address, tag.unwrap_or_default()))
.await
.map(|count: U64| count.to::<u64>())
}

Where automatically defaulting means Latest:

impl Default for BlockId {
fn default() -> Self {
BlockId::Number(BlockNumberOrTag::Latest)
}
}

Based on feedback from @mattsse on the PR of @EmperorOrokuSaki it was decided to instead of making get_code_at and get_uncle have Option<BlockId> to change all other methods to use BlockId (not optional).

Additional context

No response

@zerosnacks zerosnacks added the enhancement New feature or request label Apr 11, 2024
@EmperorOrokuSaki
Copy link
Contributor

I'll work on this.

@EmperorOrokuSaki
Copy link
Contributor

Hey, update on this. The direction is going to be the removal of all Option params and making them required in provider methods. This ensures consistency with the JSON RPC specification.

@zerosnacks
Copy link
Member Author

Hey, update on this. The direction is going to be the removal of all Option params and making them required in provider methods. This ensures consistency with the JSON RPC specification.

Thanks! I'll update the description and title

@zerosnacks zerosnacks changed the title [Feature] Support optional BlockId for get_code_at and get_uncle on the Provider [Feature] Change all Option<BlockId> to BlockId on methods in the Provider Apr 12, 2024
@zerosnacks zerosnacks changed the title [Feature] Change all Option<BlockId> to BlockId on methods in the Provider [Feature] Change all Option<BlockId> to BlockId on methods in the Provider Apr 12, 2024
@EmperorOrokuSaki
Copy link
Contributor

Can we close this issue? The PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants