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

fix: remove app-layer usage of transport error #363

Merged
merged 4 commits into from
Mar 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/json-rpc/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ pub enum RpcError<E, ErrResp = Box<RawValue>> {
#[error("server returned an error response: {0}")]
ErrorResp(ErrorPayload<ErrResp>),

/// Server returned a null response when a non-null response was expected.
#[error("server returned a null response when a non-null response was expected")]
NullResp,

/// Rpc server returned an unsupported feature.
#[error("unsupported feature: {0}")]
UnsupportedFeature(&'static str),

/// JSON serialization error.
#[error("serialization error: {0}")]
SerError(
Expand Down Expand Up @@ -94,6 +102,11 @@ impl<E, ErrResp> RpcError<E, ErrResp> {
matches!(self, Self::ErrorResp(_))
}

/// Check if the error is a null response.
pub const fn is_null_resp(&self) -> bool {
matches!(self, Self::NullResp)
}

/// Fallible conversion to an error response.
pub const fn as_error_resp(&self) -> Option<&ErrorPayload<ErrResp>> {
match self {
Expand Down
4 changes: 3 additions & 1 deletion crates/provider/src/heart.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Block heartbeat and pending transaction watcher.

use crate::{Provider, RootProvider};
use alloy_json_rpc::RpcError;
use alloy_network::Network;
use alloy_primitives::{B256, U256};
use alloy_rpc_types::Block;
Expand Down Expand Up @@ -182,7 +183,8 @@ impl<'a, N: Network, T: Transport + Clone> PendingTransactionBuilder<'a, N, T> {
let pending_tx = self.provider.watch_pending_transaction(self.config).await?;
let hash = pending_tx.await?;
let receipt = self.provider.get_transaction_receipt(hash).await?;
receipt.ok_or_else(TransportErrorKind::missing_receipt)

receipt.ok_or(RpcError::NullResp)
}
}

Expand Down
13 changes: 3 additions & 10 deletions crates/provider/src/layers/gas.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
utils::Eip1559Estimation, PendingTransactionBuilder, Provider, ProviderLayer, RootProvider,
};
use alloy_json_rpc::RpcError;
use alloy_network::{Network, TransactionBuilder};
use alloy_primitives::U256;
use alloy_transport::{Transport, TransportError, TransportResult};
Expand Down Expand Up @@ -122,16 +123,8 @@ where
tx.set_max_priority_fee_per_gas(eip1559_fees.max_priority_fee_per_gas);
Ok(())
}
Err(err) => {
if err.is_transport_error()
&& err.to_string() == *"EIP-1559 not activated".to_string()
{
// If EIP-1559 is not activated, it will process as a legacy tx.
self.handle_legacy_tx(tx).await
} else {
Err(err)
}
}
Err(RpcError::UnsupportedFeature("eip1559")) => self.handle_legacy_tx(tx).await,
Copy link
Member

Choose a reason for hiding this comment

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

I think this could also fail with a
-32601, message: Method not found

rpc error for the fee_history call?

some providers also return -32600, message: Unsupported method:

so perhaps we should be less restrictive here and also check the rpc error response?

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 could assume any error response is also a reason to retry as a legacy

i'm not 100% convinced that retrying as legacy is the correct thing to do here, as forcing legacy could conflict with users who have already put blobs onto their transaction request. We may want to just return errors here

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, so the unsupported feature check is okay because this looks at the base fee value of the header

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 need to write down somewhere that 1559 is somewhat privileged by the library in ways that other tx types and HF features are not. I.e. we generally assume that 1559 is active and proactively attempt to use it in gas estimation even tho we acknowledge that there may be chains that don't support it

Err(e) => Err(e),
}
}

Expand Down
48 changes: 16 additions & 32 deletions crates/provider/src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
utils::{self, Eip1559Estimation, EstimatorFunction},
PendingTransactionBuilder,
};
use alloy_json_rpc::{RpcParam, RpcReturn};
use alloy_json_rpc::{RpcError, RpcParam, RpcReturn};
use alloy_network::{Network, TransactionBuilder};
use alloy_primitives::{
hex, Address, BlockHash, BlockNumber, Bytes, StorageKey, StorageValue, TxHash, B256, U256, U64,
Expand Down Expand Up @@ -762,41 +762,25 @@ pub trait Provider<N: Network, T: Transport + Clone = BoxTransport>: Send + Sync
&self,
estimator: Option<EstimatorFunction>,
) -> TransportResult<Eip1559Estimation> {
let base_fee_per_gas = match self.get_block_by_number(BlockNumberOrTag::Latest, false).await
{
Ok(Some(block)) => match block.header.base_fee_per_gas {
Some(base_fee_per_gas) => base_fee_per_gas,
None => return Err(TransportErrorKind::custom_str("EIP-1559 not activated")),
},

Ok(None) => return Err(TransportErrorKind::custom_str("Latest block not found")),

Err(err) => return Err(err),
};

let fee_history = match self
.get_fee_history(
let (bf, fee_history) = futures::try_join!(
self.get_block_by_number(BlockNumberOrTag::Latest, false),
self.get_fee_history(
U256::from(utils::EIP1559_FEE_ESTIMATION_PAST_BLOCKS),
BlockNumberOrTag::Latest,
&[utils::EIP1559_FEE_ESTIMATION_REWARD_PERCENTILE],
)
.await
{
Ok(fee_history) => fee_history,
Err(err) => return Err(err),
};

// use the provided fee estimator function, or fallback to the default implementation.
let (max_fee_per_gas, max_priority_fee_per_gas) = if let Some(es) = estimator {
es(base_fee_per_gas, &fee_history.reward.unwrap_or_default())
} else {
utils::eip1559_default_estimator(
base_fee_per_gas,
&fee_history.reward.unwrap_or_default(),
)
};

Ok(Eip1559Estimation { max_fee_per_gas, max_priority_fee_per_gas })
)?;

let base_fee_per_gas = bf
.ok_or(RpcError::NullResp)?
.header
.base_fee_per_gas
.ok_or(RpcError::UnsupportedFeature("eip1559"))?;
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense


Ok(estimator.unwrap_or(utils::eip1559_default_estimator)(
base_fee_per_gas,
&fee_history.reward.unwrap_or_default(),
))
}

/// Get the account and storage values of the specified account including the merkle proofs.
Expand Down
17 changes: 12 additions & 5 deletions crates/provider/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ pub const EIP1559_BASE_FEE_MULTIPLIER: f64 = 2.0;
pub const EIP1559_FEE_ESTIMATION_REWARD_PERCENTILE: f64 = 20.0;

/// An estimator function for EIP1559 fees.
pub type EstimatorFunction = fn(U256, &[Vec<U256>]) -> (U256, U256);
pub type EstimatorFunction = fn(U256, &[Vec<U256>]) -> Eip1559Estimation;

/// Return type of EIP1155 gas fee estimator.
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Eip1559Estimation {
/// The base fee per gas.
pub max_fee_per_gas: U256,
Expand Down Expand Up @@ -42,10 +42,14 @@ fn estimate_priority_fee(rewards: &[Vec<U256>]) -> U256 {

/// The default EIP-1559 fee estimator which is based on the work by [MetaMask](https://github.com/MetaMask/core/blob/main/packages/gas-fee-controller/src/fetchGasEstimatesViaEthFeeHistory/calculateGasFeeEstimatesForPriorityLevels.ts#L56)
/// (constants for "medium" priority level are used)
pub fn eip1559_default_estimator(base_fee_per_gas: U256, rewards: &[Vec<U256>]) -> (U256, U256) {
pub fn eip1559_default_estimator(
base_fee_per_gas: U256,
rewards: &[Vec<U256>],
) -> Eip1559Estimation {
let max_priority_fee_per_gas = estimate_priority_fee(rewards);
let potential_max_fee = base_fee_per_gas * U256::from(EIP1559_BASE_FEE_MULTIPLIER);
(potential_max_fee, max_priority_fee_per_gas)

Eip1559Estimation { max_fee_per_gas: potential_max_fee, max_priority_fee_per_gas }
}

#[cfg(test)]
Expand Down Expand Up @@ -88,7 +92,10 @@ mod tests {
];
assert_eq!(
super::eip1559_default_estimator(base_fee_per_gas, &rewards),
(U256::from(2_000_000_000_u64), U256::from(200_000_000_000_u64))
Eip1559Estimation {
max_fee_per_gas: U256::from(2_000_000_000_u64),
max_priority_fee_per_gas: U256::from(200_000_000_000_u64)
}
);
}
}
9 changes: 0 additions & 9 deletions crates/transport/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ pub enum TransportErrorKind {
#[error("subscriptions are not available on this provider")]
PubsubUnavailable,

/// Transaction confirmed but `get_transaction_receipt` returned `None`.
#[error("transaction confirmed but receipt returned was null")]
MissingReceipt,

/// Custom error.
#[error("{0}")]
Custom(#[source] Box<dyn StdError + Send + Sync + 'static>),
Expand Down Expand Up @@ -71,9 +67,4 @@ impl TransportErrorKind {
pub const fn pubsub_unavailable() -> TransportError {
RpcError::Transport(Self::PubsubUnavailable)
}

/// Instantiate a new `TransportError::MissingReceipt`.
pub const fn missing_receipt() -> TransportError {
RpcError::Transport(Self::MissingReceipt)
}
}
Loading