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 eth_getTransactionByHash support #4577

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

agostbiro
Copy link
Member

Add eth_getTransactionByHash to the EDR provider:

  • Follows Hardhat RPC logic.
  • The provider implementation is tested, the RPC logic is waiting to be tested with Hardhat integration tests once those are wired in.
  • Some tangential, but necessary changes included.

Copy link

changeset-bot bot commented Nov 9, 2023

⚠️ No Changeset found

Latest commit: 5c3596a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@agostbiro agostbiro self-assigned this Nov 9, 2023
Copy link

vercel bot commented Nov 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 9, 2023 8:40pm
hardhat-storybook ✅ Ready (Inspect) Visit Preview Nov 9, 2023 8:40pm

skip_serializing_if = "Option::is_none",
with = "crate::serde::optional_u64"
)]
pub transaction_type: Option<u64>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to make it an Option in case we're running in pre-Berlin mode.

@@ -72,7 +72,7 @@ impl ProviderData {
network_id: config.network_id,
beneficiary: config.coinbase,
// TODO: Add config option (https://github.com/NomicFoundation/edr/issues/111)
min_gas_price: U256::MAX,
min_gas_price: U256::from(1),
Copy link
Member Author

@agostbiro agostbiro Nov 9, 2023

Choose a reason for hiding this comment

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

Had to decrease this to let us mine transactions in tests.

use tempfile::TempDir;

use super::*;
use crate::{test_utils::create_test_config_with_impersonated_accounts, ProviderConfig};

struct NodeTestFixture {
struct ProviderTestFixture {
Copy link
Member Author

Choose a reason for hiding this comment

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

Did a rename as this slipped by in review previously

@@ -763,7 +830,8 @@ mod tests {
vec![impersonated_account],
);

let mut provider_data = ProviderData::new(runtime, &config).await?;
let runtime = runtime::Handle::try_current()?;
let mut provider_data = ProviderData::new(&runtime, &config).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it so that we pass in the current runtime handle here instead of on initialization to avoid repetition, since this fixture is only used here.

.transpose()
}

fn get_transaction_result_to_rpc_result(
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 might want to introduce a separate module at some point for these RPC conversions, but seemed like an overkill for now.

@@ -23,43 +23,39 @@ pub use self::{
};

/// A JSON-RPC provider for Ethereum.
///
/// Add a layer in front that handles this
Copy link
Member Author

Choose a reason for hiding this comment

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

Made this a doc comment to prevent rustfmt from breaking the lines.

Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

To expedite merging, if this feedback is resolved by borrowing the SignedTransaction, feel free to merge the PR - after applying that improvement.

The other topic might take some time for the others to respond to and can be handled in a follow-up PR.

@agostbiro agostbiro merged commit 37d7fc3 into edr/main Nov 10, 2023
@agostbiro agostbiro deleted the edr/feat/get-transaction-by-hash branch November 10, 2023 16:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Port eth_getTransaction* getter RPC methods
3 participants