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: GetAccountMode #23

Closed
wants to merge 4 commits into from
Closed

feat: GetAccountMode #23

wants to merge 4 commits into from

Conversation

yash-atreya
Copy link
Member

Motivation

Closes #9

Solution

Introduces

enum GetAccountMode {
    EthGetAccount, // If the provider supports `eth_getAccount`
    AccountCodeNonce, // If it doesn't and we have to fetch all three concurrently.
}

Above enum is cached in the BackendHandler inside a OnceLock as it only needs to set exactly once i.e on the first request when we try to determine whether the provider supports eth_getAccount or not.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Comment on lines +142 to +143
/// Marker identifying whether the RPC provider supports `eth_getAccount`
get_account_mode: OnceLock<GetAccountMode>,
Copy link
Member

Choose a reason for hiding this comment

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

why does this have to be a oncelock?

Copy link
Member Author

@yash-atreya yash-atreya Sep 27, 2024

Choose a reason for hiding this comment

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

We only want to set it once on the first request, and we don't need to make get_account_req &mut self

Copy link
Member Author

Choose a reason for hiding this comment

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

We're also setting the value inside Box::pin async fut

src/backend.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

I need to test this a bit because I think especially on reth this request is more expensive than doing multiple ones because of the storage root

@mattsse
Copy link
Member

mattsse commented Oct 7, 2024

Actually we shouldn't do this because this endpoint can be significantly more expensive due to the storage root which isn't trivial to compute by certain clients

@mattsse mattsse closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use eth_getAccount if provided by the endpoint
2 participants